From 8aaceba29f75680134fb7aa23b354dc29bfb9f9d Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Fri, 6 Nov 2026 05:26:50 +0100 Subject: [PATCH 1/5] collection: Nav3 Lifecycle From ca37f8d95cafa4f878addc57b6026ae300415767 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Wed, 11 Nov 2026 12:11:20 +0100 Subject: [PATCH 2/5] ref(android): Introduce NavRootInfo and move the Nav3 lease policy to the README (#6481) Prepares the Nav3 lifecycle stack without changing behavior. SentryNavEffect now attaches a NavRootInfo (the nav root's lifecycle plus the Activity that displays it), which later lease withdrawal needs. Renames LeasedNavState to INavStatePublisher, publisher to lastPublisher, and hasAppliedInitialBackStackUpdate to hasHandledFirstUpdate/canReceiveLease, and says "nav root" rather than "owner" throughout. The lease policy (tradeoff, handoff, diagram) moves from the NavLeaseCoordinator KDoc to the package README, so later changes to the policy are reviewed as prose. --- sentry-android-navigation3/build.gradle.kts | 1 + .../compose/navigation3/BackStackObserver.kt | 14 +- .../CoordinatedBackStackObserver.kt | 31 +- .../compose/navigation3/SentryNavEffect.kt | 19 +- .../lifecycle/NavLeaseCoordinator.kt | 439 ++++++------------ .../navigation3/lifecycle/NavRootInfo.kt | 39 ++ .../navigation3/lifecycle/NavScopes.kt | 26 +- .../compose/navigation3/lifecycle/README.md | 136 +++++- .../navigation3/BackStackObserverTest.kt | 3 +- .../CoordinatedBackStackObserverTest.kt | 18 +- .../navigation3/lifecycle/FindActivityTest.kt | 56 +++ .../lifecycle/NavLeaseCoordinatorTest.kt | 46 +- .../navigation3/lifecycle/NavScopeTest.kt | 4 +- .../lifecycle/NavScopesIntegrationTest.kt | 4 +- .../navigation3/lifecycle/NavScopesTest.kt | 58 +-- 15 files changed, 507 insertions(+), 387 deletions(-) create mode 100644 sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavRootInfo.kt create mode 100644 sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/FindActivityTest.kt diff --git a/sentry-android-navigation3/build.gradle.kts b/sentry-android-navigation3/build.gradle.kts index 95462ad61f1..7ad51b43904 100644 --- a/sentry-android-navigation3/build.gradle.kts +++ b/sentry-android-navigation3/build.gradle.kts @@ -64,6 +64,7 @@ dependencies { implementation(libs.androidx.lifecycle.runtime.compose) compileOnly(libs.androidx.compose.runtime) + compileOnly(libs.androidx.compose.ui) testImplementation(libs.androidx.compose.ui.test.junit4) testImplementation(libs.androidx.fragment.ktx) diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt index 2c4fd1fb14d..0becec8b705 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt @@ -33,8 +33,8 @@ private const val NAVIGATION_OP: String = "navigation" * * **Entry identity determines whether the top has changed** * - * Referential equality (===), not structural equality, is used to determine whether the top of the - * incoming back stack has changed. That approach: + * Referential equality (===) is used to determine whether the top of the incoming back stack has + * changed. That approach: * * - matches the typical Nav3 SnapshotStateList, where an entry instance has a stable identity for * its lifetime in the stack; @@ -206,21 +206,21 @@ internal class BackStackObserver( scope.rotatePropagationContext() } - storeAsPreviousTop(currentBackStack.topEntry, currentBackStack.topEntryNormalized) + storePreviousTop(currentBackStack) } private fun handleSameTop(scope: INavScope, backStack: BackStackData) { navContext.update(scope, backStack.capturedEntriesNormalized) - storeAsPreviousTop(backStack.topEntry, backStack.topEntryNormalized) + storePreviousTop(backStack) } private fun handleEmptyBackStack(scope: INavScope) { tearDown(scope) } - private fun storeAsPreviousTop(topEntry: T, topEntryNormalized: NormalizedSentryBackStackEntry) { - previousTop = WeakReference(topEntry) - previousTopNormalized = topEntryNormalized + private fun storePreviousTop(backStack: BackStackData) { + previousTop = WeakReference(backStack.topEntry) + previousTopNormalized = backStack.topEntryNormalized } private fun clearPreviousTop() { diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserver.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserver.kt index a34a214decc..a01e658e938 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserver.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserver.kt @@ -1,15 +1,15 @@ package io.sentry.compose.navigation3 -import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleEventObserver import io.sentry.IScope import io.sentry.IScopes import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator +import io.sentry.compose.navigation3.lifecycle.NavRootInfo import io.sentry.compose.navigation3.lifecycle.NavScopes /** - * A [BackStackObserver] whose updates to a shared mutable [IScope] are coordinated with other - * `BackStackObserver`s. + * A [BackStackObserver] whose updates to a shared mutable [IScope] are coordinated with those of + * other `BackStackObserver`s. * * Coordination is achieved via [NavScopes]. * @@ -35,8 +35,8 @@ internal class CoordinatedBackStackObserver( private val navScopes = NavScopes(scopes, coordinator) private val observer = BackStackObserver(navScopes, options, entryMapper) - fun attach(lifecycle: Lifecycle) { - navScopes.attach(lifecycle) + fun attach(rootInfo: NavRootInfo) { + navScopes.attach(rootInfo) } fun onBackStackChanged(backStack: List) { @@ -52,5 +52,24 @@ internal class CoordinatedBackStackObserver( } } -/** The [NavLeaseCoordinator] shared by default by all [SentryNavEffect]s. */ +/** + * The [NavLeaseCoordinator] shared by default by all [CoordinatedBackStackObserver]s. + * + * **Lifetime** + * + * The default coordinator lives as long as the process, since lease state follows nav root + * lifecycles rather than the SDK's. Closing and re-initializing the SDK doesn't require a reset + * because: + * + * - `SentryNavEffect` resolves the current scopes on every call; + * + * - while the SDK is closed, bookkeeping continues without a scope; and + * + * - unpublishing compares by reference, so values published to an earlier scope don't affect the + * current one. + * + * Limitation: A nav transaction that's still running when the SDK is closed and re-initialized can + * be bound to the new scope the next time it's published, although its client has been closed. + * Spans then attach to it, and are lost, until it times out or the next navigation finishes it. + */ internal val defaultNavLeaseCoordinator: NavLeaseCoordinator = NavLeaseCoordinator() diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/SentryNavEffect.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/SentryNavEffect.kt index c666122efc7..41e2bda95cb 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/SentryNavEffect.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/SentryNavEffect.kt @@ -4,17 +4,19 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.remember import androidx.compose.runtime.rememberUpdatedState +import androidx.compose.ui.platform.LocalContext import androidx.lifecycle.compose.LocalLifecycleOwner import io.sentry.IScopes import io.sentry.ScopesAdapter import io.sentry.SentryOptions import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator +import io.sentry.compose.navigation3.lifecycle.NavRootInfo +import io.sentry.compose.navigation3.lifecycle.findActivity import org.jetbrains.annotations.ApiStatus /** - * An effect for generating Sentry data from your Nav3 backstack. Configure it via [options] and - * call it before you invoke your `NavDisplay`. - * + * A composable effect for generating Sentry data from your Nav3 backstack. Configure it via + * [options] and call it before you invoke your `NavDisplay`: * ```kotlin * @Composable * fun AppNavigation() { @@ -24,7 +26,7 @@ import org.jetbrains.annotations.ApiStatus * // Place SentryNavEffect in the same composable as your NavDisplay and call * // the effect first. Doing so ensures the effect's lifecycle matches your * // NavDisplay, and that any Sentry data produced by your initial nav - * // destination get attributed to the appropriate nav transaction. + * // destination are attributed to the appropriate nav transaction. * SentryNavEffect( * backStack = navBackStack, * backStackEntryMapper = { entry -> @@ -105,6 +107,7 @@ internal fun SentryNavEffect( ) { val currentBackStackEntryMapper = rememberUpdatedState(backStackEntryMapper) val lifecycle = LocalLifecycleOwner.current.lifecycle + val context = LocalContext.current val observer = remember(scopes, options, lifecycle, coordinator) { @@ -116,9 +119,13 @@ internal fun SentryNavEffect( ) } - // Register first so the initial prepared back stack update can acquire navigation ownership. + // Intentionally don't key on context. It's only used to find the Activity that displays the nav + // root, which can't change without the lifecycle changing too, and that replaces the observer. + // The context's identity can change more often (e.g., if the host app provides a new + // ContextThemeWrapper on every recomposition), and re-attaching for those changes would + // needlessly finish the nav transaction. DisposableEffect(observer) { - observer.attach(lifecycle) + observer.attach(NavRootInfo(lifecycle, context.findActivity())) onDispose { observer.dispose() } } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt index 15e3706ad4c..492a7c35526 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt @@ -15,205 +15,85 @@ import io.sentry.IScopes * Manages a lease in order to determine which [NavScopes] instance can bind nav transactions and * publish nav data to a shared [IScope]. * - * `NavScopes` instances register as [candidates][Candidate] for lease ownership via [register]. - * Each candidate observes its owner's lifecycle until it's disposed. + * `NavScopes` instances register as [candidates][Candidate] for lease ownership via [register], + * each on behalf of its [nav root][NavRootInfo]. Each candidate observes its nav root's lifecycle + * until it's disposed. * - * Note: "Owner" below refers to the `LifecycleOwner` whose lifecycle the candidate observes. - * - * **A fundamental tradeoff** - * - * During `LifecycleOwner` overlap, the nav transaction and scoped nav data can reflect - * **_either_**: - * - * 1. the outgoing owner that's still visible, or - * 2. the incoming owner that may crash before it becomes visible, - * - * but not both. This class opts for (2) so that the incoming destination populates crash context - * and any work performed is associated with it† – the idea being that start-up logic is more likely - * to cause a crash or produce interesting spans than an outgoing destination's steady state. - * - * † Associating work with the incoming transaction means spans produced by the outgoing owner may - * appear as if they were produced by the incoming owner (or vice versa, were our policy reversed). - * - * **Lease handoff policy** - * - * The lease is acquired preemptively by an incoming owner's `NavScopes` instance when it applies - * its first [back stack update][onBackStackUpdate].† - * - * A displaced owner remains eligible for lease restoration until it - * [stops][Lifecycle.Event.ON_STOP]. It gets the lease back when it - * [resumes][Lifecycle.Event.ON_RESUME], or sooner if the new owner leaves or pauses while the - * displaced owner is still visible. If restored, the displaced owner's nav data and unfinished - * transaction are reinstated. - * - * † There are some exceptions, e.g., the leaseholder is resumed and the incoming owner isn't (e.g., - * a preloaded ViewPager2 Fragment page next to the current one). Such an owner acquires the lease - * when it resumes instead. - * - * The following diagram illustrates how handoff works: - * ``` - * Time - * │ - * ▼ - * - * NavScopes A + Lifecycle A Shared state NavScopes B + Lifecycle B - * ───────────────────────── ──────────── ───────────────────────── - * - * attach() Navigation lease: none - * │ - * Initial back stack update - * │ - * ├── acquire lease ────────► Navigation lease: A - * ├── create & bind txA ────► Scope transaction: txA - * ├── publish nav data ─────► Published data: A - * │ - * ON_RESUME A is eligible for restoration - * │ - * ON_PAUSE - * │ - * ├────────────────────────► Navigation lease: A (no other resumed owner) - * │ A remains eligible for restoration - * │ - * │ attach() - * │ │ - * │ Navigation lease: A ◄────────────────────┤ - * │ Scope transaction: txA │ - * │ Published data: A │ - * │ B registered, awaiting initial back │ - * │ stack update │ - * │ │ - * │ Initial back stack update - * │ │ - * │ Suspend & unbind txA ◄───────────────────┤ - * │ Navigation lease: B │ - * │ Published data: B │ - * │ Scope transaction: txB │ - * │ (rollback point) │ - * │ │ - * │ ON_RESUME - * │ │ - * │ B eligible for restoration ◄─────────────┤ - * │ │ - * ON_STOP │ - * │ │ - * ├── retain txA; timers continue │ - * ├────────────────────────► A no longer eligible for restoration │ - * │ │ - * ON_DESTROY │ - * │ │ - * ├── finish retained txA if unfinished │ - * └────────────────────────► A no longer tracked │ - * ON_PAUSE - * │ - * Navigation lease: B (no other resumed ◄─┤ - * owner) │ - * │ - * ON_STOP - * │ - * Navigation lease: B (no other resumed ◄─┤ - * owner); B keeps recording navigation │ - * B no longer eligible for restoration │ - * │ - * ON_DESTROY - * │ - * Finish txB if unfinished; ◄──────────────┤ - * unbind if still bound │ - * Clear B's remaining screen/back stack │ - * Retain recorded nav events │ - * Navigation lease: none │ - * ▼ - * - * Alternative at rollback point: B disposes before A stops - * Unbind txB ◄─────────────── dispose B ───┤ - * Navigation lease: A │ - * Republish A's staged data │ - * Restore txA if unfinished and scope free │ - * Finish txB if unfinished │ - * ``` - * - * **Nav transactions follow navigation, not the lease** - * - * A candidate gets a nav transaction for a destination it navigates to, never merely for acquiring - * the lease. A destination reached without the lease gets its transaction when the lease is next - * acquired, if it can start then; otherwise it gets none. On reacquiring the lease without having - * navigated in the meantime, a candidate only gets its unfinished transaction back, if any. - * - * Hence the asymmetry for, e.g., ViewPager2 pages: the first time the user swipes to a preloaded - * page, it gets the transaction for the destination it reached while offscreen, but swiping back to - * a page whose transaction has finished starts none. + * See the package README for the lease policy this class enforces. * * **Exception policy** * - * Methods in this class with an `IScope` parameter can throw arbitrary exceptions†, which propagate - * to the caller. Each method updates its own bookkeeping before calling into the scope or a - * candidate's [LeasedNavState], so the coordinator remains usable if the exception is caught. - * Candidates call these methods from [configureScope][IScopes.configureScope] callbacks, whose - * error handling catches the exceptions. + * Methods in this class with an `IScope` parameter can throw arbitrary exceptions, which propagate + * to the caller. For instance, `IScope` can throw arbitrary exceptions when scope mutators notify + * registered observers – including the host app – in an unguarded loop, or when starting a nav + * transaction triggers the host app's callbacks. * - * † E.g., `IScope` can throw arbitrary exceptions when scope mutators notify registered observers – - * including the host app – in an unguarded loop, or when starting a nav transaction triggers the - * host app's callbacks. + * Each method updates its own bookkeeping so that the coordinator remains usable if the exception + * is caught by the caller. * * **Thread safety** * - * This class is ***not*** thread-safe. All `NavScopes` instances using this coordinator must - * confine their access to the same thread. + * This class is ***not*** thread-safe. All callers should confine their access to the same thread. */ @Suppress("TooManyFunctions") internal class NavLeaseCoordinator { + /** A list of all possible leaseholders. */ private val candidates = mutableListOf() - /** Candidate whose nav state was most recently published to the shared scope. */ - private var publisher: Candidate? = null - + /** The actual leaseholder. */ private var leaseHolder: Candidate? = null + /** The last candidate to publish nav state, even if it no longer holds the lease. */ + private var lastPublisher: Candidate? = null + /** - * Counter that orders candidates by when this coordinator most recently observed them in resumed - * state. + * Counter that lets us order candidates by when this coordinator most recently observed them in + * resumed state. */ private var resumeOrder: Long = 0 /** - * Registers a candidate for the nav lease that observes [lifecycle] until it's - * [disposed][NavLeaseRegistration.dispose] or `lifecycle` is destroyed. Returns `null` if - * `lifecycle` is already [destroyed][Lifecycle.State.DESTROYED]. + * Registers a [nav root][rootInfo] as a lease candidate until disposal or lifecycle destruction. + * + * Returns `null` if the lifecycle is already destroyed. */ fun register( - lifecycle: Lifecycle, scopes: IScopes, - navState: LeasedNavState, + rootInfo: NavRootInfo, + publisher: INavStatePublisher, ): NavLeaseRegistration? { + val lifecycle = rootInfo.lifecycle val lifecycleState = lifecycle.currentState + if (lifecycleState == Lifecycle.State.DESTROYED) { return null } val initialResumeOrder = if (lifecycleState == Lifecycle.State.RESUMED) ++resumeOrder else 0 - val candidate = Candidate(lifecycle, scopes, navState, lifecycleState, initialResumeOrder) - candidates += candidate + val candidate = Candidate(rootInfo, lifecycleState, initialResumeOrder, publisher, scopes) + // Only list the candidate once addObserver() succeeds, so a failure can't leave behind a + // candidate that never observes its nav root's lifecycle. lifecycle.addObserver(candidate) + candidates += candidate return candidate } /** - * Records that [candidate] is about to apply a back-stack update and acquires the nav lease if - * the owner's lifecycle permits it. - * - * Later calls for the same [candidate] do nothing. + * Gives [candidate] one chance to acquire the lease on its first back-stack update, and no-ops + * otherwise. */ private fun onBackStackUpdate(candidate: Candidate, scope: IScope) { - if (candidate.hasAppliedInitialBackStackUpdate) { + if (candidate.hasHandledFirstUpdate) { return } - candidate.hasAppliedInitialBackStackUpdate = true + candidate.hasHandledFirstUpdate = true val holder = leaseHolder - // An owner that isn't resumed doesn't take the lease from a resumed leaseholder, as it isn't + // A nav root that isn't resumed doesn't take the lease from a resumed leaseholder, as it isn't // the one the user is interacting with (e.g., a preloaded ViewPager2 page next to the current // page). Instead, it acquires the lease when it resumes. val wouldDisplaceResumedHolder = @@ -226,18 +106,68 @@ internal class NavLeaseCoordinator { } } + /** Hands the lease to [next], or releases it if [scope] is unavailable. */ + private fun handOffLease(next: Candidate?, scope: IScope?) { + if (scope != null) { + transferLeaseTo(next, scope) + } else { + leaseHolder = null + } + } + + /** Hands the lease to [candidate], or releases it given `null`. */ + private fun transferLeaseTo(candidate: Candidate?, scope: IScope) { + if (leaseHolder === candidate) { + return + } + + val previous = lastPublisher + leaseHolder = candidate + if (candidate != null) { + // Record the candidate as the last publisher before calling into the scope: it owns the + // scope's nav state from the moment it takes the lease. If unpublishing the previous nav + // root's state then throws, the next handoff still unpublishes the candidate's state rather + // than never unpublishing it. + lastPublisher = candidate + if (previous != null && previous !== candidate) { + previous.publisher.unpublishFrom(scope) + } + candidate.publisher.publishTo(scope) + } + } + + /** + * The eligible candidate most recently observed to be in a resumed state, other than [excluding]. + * + * If [resumedOnly] is set to `true`, only candidates that are resumed right now are considered; + * if `false`, started and paused candidates are also included. + */ + private fun mostRecentlyResumedCandidate( + excluding: Candidate? = null, + resumedOnly: Boolean = false, + ): Candidate? = + candidates + .filter { + it !== excluding && + it.canReceiveLease && + it.state.isAtLeast(Lifecycle.State.STARTED) && + it.resumeOrder > 0 && + (!resumedOnly || it.state == Lifecycle.State.RESUMED) + } + .maxByOrNull { it.resumeOrder } + /** * Records that [candidate] resumed. * - * Restores the lease to `candidate` if it already applied a back stack update and [scope] is - * non-null. + * Gives [candidate] the lease if its back stack has had an initial update and [scope] is + * available. */ private fun resumeCandidate(candidate: Candidate, scope: IScope?) { // A null scope (e.g., the SDK is disabled) still records the lifecycle transition so that the // candidate remains eligible for fallback once the scope becomes available. candidate.resume(++resumeOrder) - if (scope != null && candidate.hasAppliedInitialBackStackUpdate) { + if (scope != null && candidate.canReceiveLease) { transferLeaseTo(candidate, scope) } } @@ -245,15 +175,15 @@ internal class NavLeaseCoordinator { /** * Records that [candidate] paused. * - * Hands the lease to another owner only if that owner is resumed (e.g., a Fragment whose dialog - * is being dismissed); otherwise keeps it. + * If [candidate] holds the lease when it pauses, hands it to another resumed nav root (e.g., a + * Fragment whose dialog is being dismissed); otherwise the paused nav root keeps the lease. */ private fun pauseCandidate(candidate: Candidate, scope: IScope?) { candidate.pause() - // A paused leaseholder may still be the topmost visible owner (e.g., beneath a translucent - // window or picture-in-picture mode), and any other paused owner is beneath it. A displaced - // owner will acquire the lease again when it resumes. + // A paused leaseholder may still be the topmost visible nav root (e.g., beneath a translucent + // window or picture-in-picture mode), and any other paused nav root is beneath it. A displaced + // nav root will acquire the lease again when it resumes. if (leaseHolder === candidate) { mostRecentlyResumedCandidate(excluding = candidate, resumedOnly = true)?.let { next -> handOffLease(next, scope) @@ -264,93 +194,35 @@ internal class NavLeaseCoordinator { /** * Records that [candidate] stopped. * - * As with pausing, hands the lease to another owner only if that owner is resumed; otherwise the - * stopped owner keeps it. Keeping it lets a lone owner go on recording navigation that happens - * while it isn't resumed (e.g., while the app is in the background, or after it restarts but - * before it resumes), including starting nav transactions. + * If [candidate] holds the lease when it stops, hands it to another resumed nav root or keeps it + * so a lone nav root can record navigation while the app is backgrounded. */ private fun stopCandidate(candidate: Candidate, scope: IScope?) { candidate.stop() - // A paused owner doesn't get the lease. When the leaseholder stops before it, the app is - // typically going to the background (e.g., a translucent leaseholder stops before the owner - // beneath it), and that owner isn't what the user last saw. A paused owner that comes back to - // the foreground will acquire the lease when it resumes. + // A paused nav root doesn't get the lease. When the leaseholder stops before it, the app is + // typically going to the background (e.g., a translucent leaseholder stops before the nav root + // beneath it), and that nav root isn't what the user last saw. A paused nav root that comes + // back to the foreground will acquire the lease when it resumes. if (leaseHolder === candidate) { mostRecentlyResumedCandidate(resumedOnly = true)?.let { next -> handOffLease(next, scope) } } } /** - * The eligible candidate most recently observed to be in a resumed state, other than [excluding]. - * - * If [resumedOnly] is set to `true`, only candidates that are resumed right now are considered; - * if `false`, started and paused candidates are also considered. - */ - private fun mostRecentlyResumedCandidate( - excluding: Candidate? = null, - resumedOnly: Boolean = false, - ): Candidate? = - candidates - .filter { - it !== excluding && - it.hasAppliedInitialBackStackUpdate && - it.state.isAtLeast(Lifecycle.State.STARTED) && - it.resumeOrder > 0 && - (!resumedOnly || it.state == Lifecycle.State.RESUMED) - } - .maxByOrNull { it.resumeOrder } - - /** - * Hands the current lease to [next]. Without a [scope], the lease is released instead, as [next] - * could not publish its staged data. - */ - private fun handOffLease(next: Candidate?, scope: IScope?) { - if (scope != null) { - transferLeaseTo(next, scope) - } else { - leaseHolder = null - } - } - - /** Hands the lease to [candidate], or releases it given `null`. */ - private fun transferLeaseTo(candidate: Candidate?, scope: IScope) { - if (leaseHolder === candidate) { - return - } - - val previousPublisher = publisher - leaseHolder = candidate - if (candidate != null) { - // Record the candidate as the publisher before calling into the scope: it owns the scope's - // nav state from the moment it takes the lease. If unpublishing the previous owner's state - // then throws, the next handoff still unpublishes the candidate's state rather than never - // unpublishing it. - publisher = candidate - if (previousPublisher != null && previousPublisher !== candidate) { - previousPublisher.navState.unpublishFrom(scope) - } - candidate.navState.publishTo(scope) - } - } - - /** - * Stops tracking [candidate] (e.g., because it was destroyed). - * - * Unpublishes `candidate`'s nav state from [scope] if it's the most recently published. If - * `candidate` holds the lease, hands it to the eligible candidate most recently observed resumed, - * even if that candidate is now only started or paused, or releases it if there's none. + * Removes [candidate], unpublishes its state if needed, and hands off its lease to the latest + * eligible candidate. Without [scope], releases the lease without unpublishing. */ private fun unregisterCandidate(candidate: Candidate, scope: IScope?) { - val wasPublisher = publisher === candidate + val wasLastPublisher = lastPublisher === candidate val wasLeaseHolder = leaseHolder === candidate // Remove candidate before any scope calls to ensure it's released even if cleanup or handoff // throws. candidates.remove(candidate) - if (wasPublisher) { - publisher = null + if (wasLastPublisher) { + lastPublisher = null } if (wasLeaseHolder) { leaseHolder = null @@ -362,8 +234,8 @@ internal class NavLeaseCoordinator { return } - if (wasPublisher) { - candidate.navState.unpublishFrom(scope) + if (wasLastPublisher) { + candidate.publisher.unpublishFrom(scope) } if (wasLeaseHolder) { transferLeaseTo(mostRecentlyResumedCandidate(), scope) @@ -371,44 +243,55 @@ internal class NavLeaseCoordinator { } /** - * A [LifecycleOwner] registered as a candidate for lease ownership. Relays its owner's lifecycle - * events to the coordinator until it's [disposed][dispose]. + * A [nav root][rootInfo] eligible to hold the lease managed by [NavLeaseCoordinator]. + * + * [Disposed][dispose] upon the nav root's destruction. */ private inner class Candidate( - private val lifecycle: Lifecycle, - private val scopes: IScopes, - val navState: LeasedNavState, + rootInfo: NavRootInfo, initialState: Lifecycle.State, initialResumeOrder: Long, + val publisher: INavStatePublisher, + private val scopes: IScopes, ) : NavLeaseRegistration, LifecycleEventObserver { - /** The candidate's lifecycle state, as last observed. */ + /** Most recent resume order; zero until this candidate first resumes. */ + var resumeOrder: Long = initialResumeOrder + private set + + /** The candidate's last observed lifecycle state. */ var state: Lifecycle.State = initialState private set + /** Whether this candidate has handled its first back-stack update. */ + var hasHandledFirstUpdate: Boolean = false + /** - * Whether the candidate has been observed STOPPED since - * [registration][NavLeaseCoordinator.register]. + * Whether the first back-stack update may take the lease: the nav root is resumed or has not + * yet resumed or stopped since registration. */ - private var wasStopped: Boolean = false + val canAcquireLeaseOnFirstUpdate: Boolean + get() = state == Lifecycle.State.RESUMED || (resumeOrder == 0L && !wasStopped) /** - * See [NavLeaseCoordinator.resumeOrder]. Non-zero once the candidate has been observed RESUMED - * since [registration][NavLeaseCoordinator.register]. + * Whether this candidate can acquire the lease on a lifecycle change, i.e., when it resumes or + * as a fallback. + * + * False until its first back stack update is handled, as it has no nav state to publish before + * then. */ - var resumeOrder: Long = initialResumeOrder - private set + val canReceiveLease: Boolean + get() = hasHandledFirstUpdate - var hasAppliedInitialBackStackUpdate: Boolean = false + override val hasLease: Boolean + get() = leaseHolder === this - /** - * Whether the candidate's lifecycle permits its first back stack update since - * [registration][NavLeaseCoordinator.register] to acquire the nav lease: it's resumed, or it - * has neither resumed nor stopped since registration (i.e., it's still coming up for the first - * time). - */ - val canAcquireLeaseOnFirstUpdate: Boolean - get() = state == Lifecycle.State.RESUMED || (resumeOrder == 0L && !wasStopped) + private val lifecycle: Lifecycle = rootInfo.lifecycle + + /** Whether this candidate has stopped since registration. */ + private var wasStopped: Boolean = false + + private var isDisposed = false fun start() { // Attaching to an already-resumed lifecycle replays ON_START before ON_RESUME, so only move @@ -432,11 +315,6 @@ internal class NavLeaseCoordinator { wasStopped = true } - private var isDisposed = false - - override val hasLease: Boolean - get() = leaseHolder === this - override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) { when (event) { ON_START -> start() @@ -466,17 +344,14 @@ internal class NavLeaseCoordinator { try { unregisterCandidate(this, scope) } finally { - navState.onUnregistered(scope) + publisher.onUnregistered(scope) } } } /** - * Runs [block] with the real scope, or with `null` if [configureScope][IScopes.configureScope] - * skips its callback (e.g., the SDK is disabled), so coordinator bookkeeping still happens. - * - * A callback that ran but threw is not retried, as retrying could let its exception escape into - * the host app. + * Runs [block] with the scope, or with `null` if [IScopes.configureScope] skips its callback. + * Does not rerun a callback that throws. */ private fun withScopeOrNull(block: (IScope?) -> Unit) { var callbackInvoked = false @@ -502,41 +377,25 @@ internal interface NavLeaseRegistration { /** Whether the candidate holds the nav lease. */ val hasLease: Boolean - /** - * Announces that the candidate is about to apply a back stack update, so it can acquire the nav - * lease if it's eligible to. Does nothing once the candidate is unregistered. - */ + /** Gives the candidate a chance to acquire the lease before its first back-stack update. */ fun onBackStackUpdate() /** - * Stops observing the candidate's lifecycle and unregisters it: releases the lease, unpublishes - * the candidate's nav state, and then calls [LeasedNavState.onUnregistered], even if - * unregistering throws. Does nothing once the candidate is unregistered. + * Unregisters the candidate, releases its lease, and unpublishes its nav state. Calls + * [INavStatePublisher.onUnregistered] even if cleanup throws. Repeated calls do nothing. */ fun dispose() } -/** - * The nav state a [NavLeaseCoordinator] candidate publishes to the shared [IScope] while it holds - * the nav lease. - * - * Nav transactions and nav data each occupy a single, mutually-exclusive slot in the `IScope`, so - * only one candidate's nav state can be published at a time. Unpublishing nav state removes it from - * the scope but keeps it staged, so the candidate can publish it again if it regains the lease. - */ -internal interface LeasedNavState { +/** Publishes a [NavLeaseCoordinator.Candidate]'s nav state to the shared [IScope]. */ +internal interface INavStatePublisher { - /** Publishes staged nav state to [scope]. */ + /** Publishes the candidate's staged nav state to [scope]. */ fun publishTo(scope: IScope) - /** Removes published nav state from [scope], keeping it staged so it can be republished. */ + /** Removes published nav state from [scope], retaining it for a later lease acquisition. */ fun unpublishFrom(scope: IScope) - /** - * Called when the lease candidate is unregistered, after its nav state has been unpublished. - * - * The provided [scope] is the scope the candidate was unregistered against, or `null` if there - * was none (e.g., the SDK is disabled). - */ + /** Called after unregistering, with the affected [scope] or `null` if none was available. */ fun onUnregistered(scope: IScope?) } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavRootInfo.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavRootInfo.kt new file mode 100644 index 00000000000..01f64bef0d4 --- /dev/null +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavRootInfo.kt @@ -0,0 +1,39 @@ +package io.sentry.compose.navigation3.lifecycle + +import android.app.Activity +import android.content.Context +import android.content.ContextWrapper +import androidx.lifecycle.Lifecycle + +/** + * Describes the nav root of a `SentryNavEffect`. + * + * A "nav root" is the Activity or Fragment whose content calls a `SentryNavEffect` and owns the + * back stack the effect observes. + */ +internal class NavRootInfo( + /** The nav root's lifecycle. */ + val lifecycle: Lifecycle, + /** + * The Activity that displays the nav root's content, or `null` if it's unknown. + * + * If the nav root is itself an Activity, this is the nav root. If the nav root is a Fragment, + * this is the Activity that hosts the Fragment. + */ + val activity: Activity?, +) + +/** + * The Activity the receiver belongs to, found by unwrapping [ContextWrapper]s (e.g., the + * `ContextThemeWrapper` of a dialog), or `null` if there is none. + */ +internal fun Context.findActivity(): Activity? { + var context: Context? = this + while (context is ContextWrapper) { + if (context is Activity) { + return context + } + context = context.baseContext + } + return null +} diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt index 37a796a58c6..8c1c6d32b7e 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt @@ -24,8 +24,8 @@ internal const val NAVIGATION_CONTEXT_KEY: String = "Navigation" * Needed because all `SentryNavEffect`s mutate a single [IScope] instance by default and will * clobber or interleave one another's updates unless properly managed: * ``` - * LifecycleOwner (e.g., Activity or Fragment) - * ↓ owns + * Nav root (Activity or Fragment) + * ↓ owns / calls * SentryNavEffect * ↓ * CoordinatedBackStackObserver (wrapper) @@ -56,7 +56,7 @@ internal const val NAVIGATION_CONTEXT_KEY: String = "Navigation" * 2. Pass the same [NavLeaseCoordinator] instance to all `NavScopes` whose effects must be * coordinated (i.e., that target the same `IScope`). * - * 3. Call [attach] with the lifecycle that manages the effect before the first back stack update. + * 3. Call [attach] with the effect's [nav root][NavRootInfo] before the first back stack update. * * 4. Call [onBackStackUpdate] before applying each back stack update. * @@ -86,16 +86,18 @@ internal class NavScopes( private val navStatePublisher = NavStatePublisher() /** - * Registers this [NavScopes] instance with [lifecycle]. No-ops if `lifecycle` has already been - * attached or is [destroyed][Lifecycle.Event.ON_DESTROY]. + * Registers this [NavScopes] instance with the lifecycle of the effect's nav root, described by + * [rootInfo]. * - * Attaching to a second, different lifecycle no-ops. + * No-ops if the caller tries to attach more than once or if the nav root has already been + * destroyed. */ - fun attach(lifecycle: Lifecycle) { + fun attach(rootInfo: NavRootInfo) { val currentLifecycle = attachedLifecycle + val incomingLifecycle = rootInfo.lifecycle if (currentLifecycle != null) { - if (currentLifecycle !== lifecycle) { + if (currentLifecycle !== incomingLifecycle) { delegate.options.logger.log( SentryLevel.WARNING, "NavScopes is already attached to a different lifecycle; ignoring the new lifecycle.", @@ -106,12 +108,12 @@ internal class NavScopes( return } - if (lifecycle.currentState == Lifecycle.State.DESTROYED) { + if (incomingLifecycle.currentState == Lifecycle.State.DESTROYED) { return } - attachedLifecycle = lifecycle - registration = coordinator.register(lifecycle, delegate, navStatePublisher) + attachedLifecycle = incomingLifecycle + registration = coordinator.register(delegate, rootInfo, navStatePublisher) } /** @@ -147,7 +149,7 @@ internal class NavScopes( * Publishes this instance's [stagedNavTransaction] and [stagedNavData] to the shared [IScope] * while it holds the nav lease, as directed by the [coordinator]. */ - private inner class NavStatePublisher : LeasedNavState { + private inner class NavStatePublisher : INavStatePublisher { override fun publishTo(scope: IScope) { // Publishing the data first ensures a staged transaction is sampled against the rotated diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md index e003af99061..d3e1a942bdf 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md @@ -3,10 +3,6 @@ Package contains code for coordinating multiple `SentryNavEffect`s and their access to a shared `IScope`, so that overlapping effects (e.g., during Activity or Fragment transitions) don't clobber one another's screen names, nav context, or nav transactions. -See the `NavLeaseCoordinator` KDoc for the lease policy that decides which effect may write to the scope. - -## Encapsulation policy - This package is meant to encapsulate knowledge about the possibility of multiple `SentryNavEffect`s and the shared mutable state issues it raises. Addressing those concerns is complicated. Confining them here makes them easier to reason about and easier to swap for alternative logic should we someday need to. @@ -16,10 +12,6 @@ logic should we someday need to. Support for multiple `SentryNavEffect`s is restricted to the scenarios documented in the [Navigation for Android docs](https://docs.sentry.io/platforms/android/integrations/navigation/#supported-setups). -`NavLeaseCoordinator` implements the policy that underlies those scenarios. It assumes that, at any moment, the user is interacting with at -most one registered `LifecycleOwner` in the host app, and that the `LifecycleOwner` being interacted with is the one that was most recently -resumed. - Concretely, a host app can safely use more than one `SentryNavEffect` only if: 1. each `SentryNavEffect` is called in the same composable as the `NavDisplay` it observes; @@ -28,7 +20,133 @@ Concretely, a host app can safely use more than one `SentryNavEffect` only if: not alongside one in the Activity hosting them); and 4. the user can interact with only one of those Activities or Fragments at a time. -† Meant to ensure each effect's `LocalLifecycleOwner` is an Activity or a Fragment and not a `NavDisplay`-provided owner. `NavDisplay` +> † Meant to ensure each effect's `LocalLifecycleOwner` is an Activity or a Fragment and not a `NavDisplay`-provided owner. `NavDisplay` provides its own `LocalLifecycleOwner` to the entries and scenes it renders. That owner drops to STARTED state during transitions and while overlays are shown. An effect called inside a `NavDisplay` would therefore (wrongly) gain and lose the nav lease `NavLeaseCoordinator` manages as navigation animates, rather than as its Activity or Fragment comes and goes. + +## Lease policy + +`NavLeaseCoordinator` implements the lease policy that supports the scenarios from the Navigation for Android docs. The policy assumes that, +at any moment, the user is interacting with at most one nav root in the host app, and that the nav root being interacted with is the one +that was most recently resumed. (See the `NavRootInfo` KDoc for a definition of "nav root".) + +### A fundamental tradeoff + +While nav roots overlap (e.g., during an Activity transition), the nav transaction and scoped nav data can reflect **_either_**: + +1. the outgoing nav root that's still visible, or +2. the incoming nav root that may crash before it becomes visible, + +but not both. `NavLeaseCoordinator` opts for (2) so that the incoming destination populates crash context and any work performed is +associated with it† – the idea being that start-up logic is more likely to cause a crash or produce interesting spans than an outgoing +destination's steady state. + +> † Associating work with the incoming transaction means spans produced by the outgoing nav root +may appear as if they were produced by the incoming nav root (or vice versa, were our policy +reversed). + +### Lease handoff policy + +The lease is acquired preemptively by an incoming nav root's `NavScopes` instance when it applies its first back stack update +(`onBackStackUpdate`).† + +A displaced nav root remains eligible for lease restoration until it stops (`ON_STOP`). It gets the lease back when it resumes +(`ON_RESUME`), or sooner if the new nav root leaves or pauses while the displaced nav root is still visible. If restored, the displaced nav +root's nav data and unfinished transaction are reinstated. + +> † The exception is an incoming nav root that isn't resumed while the leaseholder is, e.g., a +preloaded ViewPager2 Fragment page next to the current one. The user isn't interacting with such a +nav root yet, so it acquires the lease when it resumes instead. + +The following diagram illustrates how handoff works: + +``` + Time + │ + ▼ + +Nav root A Shared IScope state Nav root B +────────── ─────────────────── ────────── + +attach() Navigation lease: none + │ +Initial back stack update + │ + ├── acquire lease ────────► Navigation lease: A + ├── create & bind txA ────► Scope transaction: txA + ├── publish nav data ─────► Published data: A + │ +ON_RESUME A is eligible for restoration + │ +ON_PAUSE + │ + ├────────────────────────► Navigation lease: A (no other resumed nav root) + │ A remains eligible for restoration + │ + │ attach() + │ │ + │ Navigation lease: A ◄────────────────────┤ + │ Scope transaction: txA │ + │ Published data: A │ + │ B registered, awaiting initial back │ + │ stack update │ + │ │ + │ Initial back stack update + │ │ + │ Suspend & unbind txA ◄───────────────────┤ + │ Navigation lease: B │ + │ Published data: B │ + │ Scope transaction: txB │ + │ (rollback point) │ + │ │ + │ ON_RESUME + │ │ + │ B eligible for restoration ◄─────────────┤ + │ │ +ON_STOP │ + │ │ + ├── retain txA; timers continue │ + ├────────────────────────► A no longer eligible for restoration │ + │ │ +ON_DESTROY │ + │ │ + ├── finish retained txA if unfinished │ + └────────────────────────► A no longer tracked │ + ON_PAUSE + │ + Navigation lease: B (no other resumed ◄─┤ + nav root) │ + │ + ON_STOP + │ + Navigation lease: B (no other resumed ◄─┤ + nav root); B keeps recording │ + navigation │ + B no longer eligible for restoration │ + │ + ON_DESTROY + │ + Finish txB if unfinished; ◄──────────────┤ + unbind if still bound │ + Clear B's remaining screen/back stack │ + Retain recorded nav events │ + Navigation lease: none │ + ▼ + +Alternative at rollback point: B disposes before A stops + Unbind txB ◄─────────────── dispose B ───┤ + Navigation lease: A │ + Republish A's staged data │ + Restore txA if unfinished and scope free │ + Finish txB if unfinished │ +``` + +### Nav transactions follow navigation, not the lease + +A nav root gets a nav transaction for a destination it navigates to, never merely for acquiring the lease. A destination reached without +the lease gets its transaction when the lease is next acquired, if it can start then; otherwise it gets none. On reacquiring the lease +without having navigated in the meantime, a nav root only gets its unfinished transaction back, if any. + +Hence the asymmetry for, e.g., ViewPager2 pages: the first time the user swipes to a preloaded page, it gets the transaction for the +destination it reached while offscreen, but swiping back to a page whose transaction has finished starts none. diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackObserverTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackObserverTest.kt index d331afabe69..ef79931174f 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackObserverTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackObserverTest.kt @@ -21,6 +21,7 @@ import io.sentry.TransactionOptions import io.sentry.compose.navigation3.ArgumentDropReason.Companion.ARGUMENT_DROP_REASON_KEY import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator +import io.sentry.compose.navigation3.lifecycle.NavRootInfo import io.sentry.compose.navigation3.lifecycle.NavScopes import io.sentry.protocol.App import io.sentry.protocol.TransactionNameSource @@ -119,7 +120,7 @@ class BackStackObserverTest { LifecycleRegistry.createUnsafe(this).apply { currentState = Lifecycle.State.RESUMED } } return NavScopes(scopes, NavLeaseCoordinator()).apply { - attach(owner.lifecycle) + attach(NavRootInfo(owner.lifecycle, activity = null)) // The sole resumed owner's first update acquires the lease, which it then keeps. onBackStackUpdate() } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt index cf653c23e31..dea07db5481 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt @@ -15,9 +15,10 @@ import io.sentry.SentryOptions import io.sentry.SentryTracer import io.sentry.TransactionContext import io.sentry.TransactionOptions -import io.sentry.compose.navigation3.lifecycle.LeasedNavState +import io.sentry.compose.navigation3.lifecycle.INavStatePublisher import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator +import io.sentry.compose.navigation3.lifecycle.NavRootInfo import org.junit.Test import org.mockito.kotlin.any import org.mockito.kotlin.doAnswer @@ -360,7 +361,14 @@ class CoordinatedBackStackObserverTest { /** Registers another owner that takes the nav lease and then pauses, keeping the lease. */ fun givenPausedLeaseholder() { val otherLifecycle = resumedLifecycle() - val other = checkNotNull(coordinator.register(otherLifecycle, scopes, NoOpLeasedNavState)) + val other = + checkNotNull( + coordinator.register( + scopes, + NavRootInfo(otherLifecycle, activity = null), + NoOpNavStatePublisher, + ) + ) other.onBackStackUpdate() otherLifecycle.currentState = Lifecycle.State.STARTED check(other.hasLease) @@ -380,7 +388,7 @@ class CoordinatedBackStackObserverTest { }, coordinator = coordinator, ) - .apply { attach(lifecycle) } + .apply { attach(NavRootInfo(lifecycle, activity = null)) } /** * Owners of the lifecycles created by [resumedLifecycle]. `LifecycleRegistry` holds its owner @@ -397,8 +405,8 @@ class CoordinatedBackStackObserverTest { .lifecycle } - /** Nav state for a competing owner that has nothing to publish. */ - private object NoOpLeasedNavState : LeasedNavState { + /** Nav state publisher for a competing nav root that has nothing to publish. */ + private object NoOpNavStatePublisher : INavStatePublisher { override fun publishTo(scope: IScope) = Unit override fun unpublishFrom(scope: IScope) = Unit diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/FindActivityTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/FindActivityTest.kt new file mode 100644 index 00000000000..1c8853155d1 --- /dev/null +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/FindActivityTest.kt @@ -0,0 +1,56 @@ +package io.sentry.compose.navigation3.lifecycle + +import android.app.Activity +import android.content.Context +import android.content.ContextWrapper +import android.view.ContextThemeWrapper +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.mock +import org.robolectric.Robolectric +import org.robolectric.annotation.Config + +@RunWith(AndroidJUnit4::class) +@Config(sdk = [30]) +class FindActivityTest { + + @Test + fun `findActivity() - returns the context when the context is an activity`() { + val activity = Robolectric.buildActivity(Activity::class.java).get() + + val found = activity.findActivity() + + assertThat(found).isSameInstanceAs(activity) + } + + @Test + fun `findActivity() - returns the wrapped activity when the context wraps an activity`() { + val activity = Robolectric.buildActivity(Activity::class.java).get() + val context = ContextWrapper(ContextThemeWrapper(activity, 0)) + + val found = context.findActivity() + + assertThat(found).isSameInstanceAs(activity) + } + + @Test + fun `findActivity() - returns null when the context wraps no activity`() { + val context: Context = ApplicationProvider.getApplicationContext() + + val found = context.findActivity() + + assertThat(found).isNull() + } + + @Test + fun `findActivity() - returns null when the context isn't a context wrapper`() { + val context = mock() + + val found = context.findActivity() + + assertThat(found).isNull() + } +} diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt index 9a521312f68..20acfc4078b 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt @@ -32,7 +32,11 @@ class NavLeaseCoordinatorTest { val owner = TestOwner(DESTROYED) val registration = - navLeaseCoordinator.register(owner.lifecycle, fixture.scopes, fixture.navState("owner")) + navLeaseCoordinator.register( + fixture.scopes, + NavRootInfo(owner.lifecycle, activity = null), + fixture.publisher("owner"), + ) assertThat(registration).isNull() assertThat(owner.lifecycle.observerCount).isEqualTo(0) @@ -221,8 +225,8 @@ class NavLeaseCoordinatorTest { fixture.acquireLease( "previous", TestOwner(RESUMED), - navState = - fixture.navState( + publisher = + fixture.publisher( "previous", onPublishTo = { calls += "publish previous" }, onUnpublishFrom = { calls += "unpublish previous" }, @@ -232,7 +236,7 @@ class NavLeaseCoordinatorTest { fixture.register( "incoming", TestOwner(RESUMED), - fixture.navState( + fixture.publisher( "incoming", onPublishTo = { calls += "publish incoming" }, onUnpublishFrom = { calls += "unpublish incoming" }, @@ -288,7 +292,7 @@ class NavLeaseCoordinatorTest { fixture.acquireLease( "first", TestOwner(RESUMED), - navState = fixture.navState("first", onUnpublishFrom = { throw failure }), + publisher = fixture.publisher("first", onUnpublishFrom = { throw failure }), ) val second = fixture.register("second", TestOwner(RESUMED)) @@ -718,7 +722,7 @@ class NavLeaseCoordinatorTest { fixture.acquireLease( "owner", owner, - navState = fixture.navState("owner", onUnpublishFrom = { throw failure }), + publisher = fixture.publisher("owner", onUnpublishFrom = { throw failure }), ) val thrown = assertThrows(IllegalStateException::class.java) { registration.dispose() } @@ -755,8 +759,8 @@ class NavLeaseCoordinatorTest { fixture.acquireLease( "owner", TestOwner(RESUMED), - navState = - fixture.navState( + publisher = + fixture.publisher( "owner", onUnpublishFrom = { calls += "unpublish" }, onUnregistered = { calls += "unregistered" }, @@ -796,7 +800,7 @@ class NavLeaseCoordinatorTest { fixture.acquireLease( "owner", TestOwner(RESUMED), - navState = fixture.navState("owner", onUnpublishFrom = { throw failure }), + publisher = fixture.publisher("owner", onUnpublishFrom = { throw failure }), ) val thrown = assertThrows(IllegalStateException::class.java) { registration.dispose() } @@ -889,24 +893,28 @@ class NavLeaseCoordinatorTest { private val owners = mutableListOf() /** - * Nav state named [name] that, by default, records its publication in [published], its + * Publisher named [name] that, by default, records its publication in [published], its * unpublishing in [unpublished], and its unregistration in [unregistered]. */ - fun navState( + fun publisher( name: String, onPublishTo: (IScope) -> Unit = { published += name }, onUnpublishFrom: (IScope) -> Unit = { unpublished += name }, onUnregistered: (IScope?) -> Unit = { unregistered += name to it }, - ): LeasedNavState = FakeLeasedNavState(onPublishTo, onUnpublishFrom, onUnregistered) + ): INavStatePublisher = FakeNavStatePublisher(onPublishTo, onUnpublishFrom, onUnregistered) - /** Registers a candidate for [owner]'s lifecycle with the nav state named [name]. */ + /** + * Registers a candidate with the publisher named [name] for a nav root whose lifecycle is + * [owner]'s. + */ fun register( name: String, owner: TestOwner, - navState: LeasedNavState = navState(name), + publisher: INavStatePublisher = publisher(name), ): NavLeaseRegistration { owners += owner - return checkNotNull(navLeaseCoordinator.register(owner.lifecycle, scopes, navState)) + val rootInfo = NavRootInfo(owner.lifecycle, activity = null) + return checkNotNull(navLeaseCoordinator.register(scopes, rootInfo, publisher)) } /** @@ -916,8 +924,8 @@ class NavLeaseCoordinatorTest { fun acquireLease( name: String, owner: TestOwner, - navState: LeasedNavState = navState(name), - ): NavLeaseRegistration = register(name, owner, navState).also { it.onBackStackUpdate() } + publisher: INavStatePublisher = publisher(name), + ): NavLeaseRegistration = register(name, owner, publisher).also { it.onBackStackUpdate() } /** Runs [block] while `configureScope` skips its callbacks, e.g., as if the SDK is disabled. */ fun withoutScope(block: () -> Unit) { @@ -934,11 +942,11 @@ class NavLeaseCoordinatorTest { } } - private class FakeLeasedNavState( + private class FakeNavStatePublisher( private val onPublishTo: (IScope) -> Unit, private val onUnpublishFrom: (IScope) -> Unit, private val onUnregistered: (IScope?) -> Unit, - ) : LeasedNavState { + ) : INavStatePublisher { override fun publishTo(scope: IScope) = onPublishTo(scope) override fun unpublishFrom(scope: IScope) = onUnpublishFrom(scope) diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt index 57be6d68d5d..ad2c7b9ae40 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt @@ -477,7 +477,9 @@ class NavScopeTest { private fun resumedNavScopes(): NavScopes = NavScopes(scopes, coordinator).also { - it.attach(TestLifecycleOwner().also(lifecycleOwners::add).lifecycle) + it.attach( + NavRootInfo(TestLifecycleOwner().also(lifecycleOwners::add).lifecycle, activity = null) + ) } companion object { diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt index 4f01197a0f7..fe20e834ae8 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt @@ -24,11 +24,11 @@ class NavScopesIntegrationTest { val fixture = Fixture.create() val incumbentOwner = TestLifecycleOwner(Lifecycle.State.RESUMED) val incumbent = fixture.navScopes() - incumbent.attach(incumbentOwner.lifecycle) + incumbent.attach(NavRootInfo(incumbentOwner.lifecycle, activity = null)) incumbent.onBackStackUpdate() val incomingOwner = TestLifecycleOwner(Lifecycle.State.STARTED) val incoming = fixture.navScopes() - incoming.attach(incomingOwner.lifecycle) + incoming.attach(NavRootInfo(incomingOwner.lifecycle, activity = null)) incoming.onBackStackUpdate() val contextBeforeResume = fixture.scope.propagationContext val contextsAtSampling = mutableListOf() diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt index c865e8e91e6..a50ec5445c0 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt @@ -36,7 +36,7 @@ class NavScopesTest { fun `attach() - lets the first back stack update acquire the nav lease`() { val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle()) + navScopes.attach(NavRootInfo(fixture.lifecycle(), activity = null)) navScopes.applyFirstBackStackUpdate() @@ -48,9 +48,9 @@ class NavScopesTest { val fixture = Fixture.create() val navScopes = fixture.navScopes() val lifecycle = fixture.lifecycle() - navScopes.attach(lifecycle) + navScopes.attach(NavRootInfo(lifecycle, activity = null)) - navScopes.attach(lifecycle) + navScopes.attach(NavRootInfo(lifecycle, activity = null)) assertThat(lifecycle.observerCount).isEqualTo(1) } @@ -61,9 +61,9 @@ class NavScopesTest { val navScopes = fixture.navScopes() val first = TestLifecycleOwner() val second = TestLifecycleOwner() - navScopes.attach(first.lifecycle) + navScopes.attach(NavRootInfo(first.lifecycle, activity = null)) - navScopes.attach(second.lifecycle) + navScopes.attach(NavRootInfo(second.lifecycle, activity = null)) assertThat(first.lifecycle.observerCount).isEqualTo(1) assertThat(second.lifecycle.observerCount).isEqualTo(0) @@ -75,7 +75,7 @@ class NavScopesTest { val navScopes = fixture.navScopes() val lifecycle = fixture.lifecycle(Lifecycle.State.DESTROYED) - navScopes.attach(lifecycle) + navScopes.attach(NavRootInfo(lifecycle, activity = null)) navScopes.applyFirstBackStackUpdate() assertThat(fixture.hasLease(navScopes)).isFalse() @@ -86,12 +86,12 @@ class NavScopesTest { val fixture = Fixture.create() val navScopes = fixture.navScopes() val first = fixture.lifecycle(Lifecycle.State.RESUMED) - navScopes.attach(first) + navScopes.attach(NavRootInfo(first, activity = null)) navScopes.applyFirstBackStackUpdate() first.currentState = Lifecycle.State.DESTROYED val second = fixture.lifecycle(Lifecycle.State.RESUMED) - navScopes.attach(second) + navScopes.attach(NavRootInfo(second, activity = null)) navScopes.applyFirstBackStackUpdate() assertThat(second.observerCount).isEqualTo(1) @@ -113,7 +113,7 @@ class NavScopesTest { val fixture = Fixture.create() val navScopes = fixture.navScopes() val lifecycle = fixture.lifecycle(Lifecycle.State.RESUMED) - navScopes.attach(lifecycle) + navScopes.attach(NavRootInfo(lifecycle, activity = null)) navScopes.applyFirstBackStackUpdate() navScopes.startNavTransaction("/Home") val started = fixture.startedTransaction("/Home") @@ -129,7 +129,7 @@ class NavScopesTest { val fixture = Fixture.create() val navScopes = fixture.navScopes() val lifecycle = fixture.lifecycle(Lifecycle.State.RESUMED) - navScopes.attach(lifecycle) + navScopes.attach(NavRootInfo(lifecycle, activity = null)) navScopes.applyFirstBackStackUpdate() navScopes.startNavTransaction("/Home") val started = fixture.startedTransaction("/Home") @@ -144,10 +144,10 @@ class NavScopesTest { fun `addBreadcrumb() - records breadcrumbs immediately without the nav lease`() { val fixture = Fixture.create() val leaseholder = fixture.navScopes() - leaseholder.attach(TestLifecycleOwner().lifecycle) + leaseholder.attach(NavRootInfo(TestLifecycleOwner().lifecycle, activity = null)) leaseholder.applyFirstBackStackUpdate() val other = fixture.navScopes() - other.attach(TestLifecycleOwner().lifecycle) + other.attach(NavRootInfo(TestLifecycleOwner().lifecycle, activity = null)) val breadcrumb = Breadcrumb() val hint = Hint() @@ -197,7 +197,7 @@ class NavScopesTest { fun `dispose() - doesn't finish the nav transaction without a scope`() { val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() navScopes.startNavTransaction("/Home") val started = fixture.startedTransaction("/Home") @@ -215,7 +215,7 @@ class NavScopesTest { navScopes.startNavTransaction("/Home") navScopes.dispose() - navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() assertThat(fixture.hasLease(navScopes)).isTrue() @@ -226,7 +226,7 @@ class NavScopesTest { fun `startNavTransaction() - starts and binds a nav transaction with the nav lease`() { val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() val transaction = navScopes.startNavTransaction("/Home") @@ -240,7 +240,7 @@ class NavScopesTest { fun `startNavTransaction() - returns null when the delegate declines with the nav lease`() { val fixture = Fixture.create(declinesTransactions = true) val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() val transaction = navScopes.startNavTransaction("/Home") @@ -252,7 +252,7 @@ class NavScopesTest { fun `startNavTransaction() - logs the route when another transaction blocks the start`() { val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() fixture.scope.transaction = mock() @@ -293,7 +293,7 @@ class NavScopesTest { fun `startNavTransaction() - starts no nav transaction after dispose`() { val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle()) + navScopes.attach(NavRootInfo(fixture.lifecycle(), activity = null)) navScopes.applyFirstBackStackUpdate() navScopes.dispose() @@ -306,7 +306,7 @@ class NavScopesTest { fun `finishNavTransaction() - finishes the started nav transaction`() { val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() navScopes.startNavTransaction("/Home") val started = fixture.startedTransaction("/Home") @@ -320,7 +320,7 @@ class NavScopesTest { fun `finishNavTransaction() - finishes the started nav transaction only once`() { val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() navScopes.startNavTransaction("/Home") val started = fixture.startedTransaction("/Home") @@ -335,7 +335,7 @@ class NavScopesTest { fun `finishNavTransaction() - does nothing without a nav transaction`() { val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() val hostTransaction = mock() fixture.scope.transaction = hostTransaction @@ -350,7 +350,7 @@ class NavScopesTest { fun `a nav transaction starts and is bound after the previous one is finished`() { val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() navScopes.startNavTransaction("/Home") navScopes.finishNavTransaction() @@ -454,11 +454,11 @@ class NavScopesTest { fun `a preloaded owner's deferred transaction starts when the owner resumes`() { val fixture = Fixture.create() val current = fixture.navScopes() - current.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + current.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) current.applyFirstBackStackUpdate() val preloaded = fixture.navScopes() val preloadedLifecycle = fixture.lifecycle(Lifecycle.State.STARTED) - preloaded.attach(preloadedLifecycle) + preloaded.attach(NavRootInfo(preloadedLifecycle, activity = null)) preloaded.applyFirstBackStackUpdate() preloaded.startNavTransaction("/PageB") check(!fixture.hasLease(preloaded)) @@ -476,7 +476,7 @@ class NavScopesTest { fixture.incoming.dispose() val started = fixture.startedTransaction("/Result") val nextIncoming = fixture.navScopes() - nextIncoming.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + nextIncoming.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) nextIncoming.applyFirstBackStackUpdate() check(fixture.scope.transaction == null) @@ -493,7 +493,7 @@ class NavScopesTest { covered.startNavTransaction("/Result") fixture.incoming.dispose() val nextIncoming = fixture.navScopes() - nextIncoming.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + nextIncoming.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) nextIncoming.applyFirstBackStackUpdate() nextIncoming.dispose() @@ -614,7 +614,7 @@ class NavScopesTest { check(fixture.startedTransactions.isEmpty()) observerThrows = false val nextIncoming = fixture.navScopes() - nextIncoming.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + nextIncoming.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) nextIncoming.applyFirstBackStackUpdate() check(!fixture.hasLease(covered)) @@ -743,7 +743,7 @@ class NavScopesTest { */ fun coveredNavScopes(startedRoute: String? = null): NavScopes { val covered = navScopes() - covered.attach(lifecycle(Lifecycle.State.RESUMED)) + covered.attach(NavRootInfo(lifecycle(Lifecycle.State.RESUMED), activity = null)) covered.onBackStackUpdate() if (startedRoute != null) { covered.configureScope { @@ -751,7 +751,7 @@ class NavScopesTest { } } incoming = navScopes() - incoming.attach(lifecycle(Lifecycle.State.RESUMED)) + incoming.attach(NavRootInfo(lifecycle(Lifecycle.State.RESUMED), activity = null)) incoming.onBackStackUpdate() check(!hasLease(covered)) return covered From f6c1977d2ac8c4af1f6ea1d460279f6a741294ef Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Thu, 12 Nov 2026 10:28:58 +0100 Subject: [PATCH 3/5] feat(android): [Part 2] Start a new nav transaction when returning to a nav root (#6482) A nav root that regains the nav lease (e.g., when the Activity covering it closes, the user swipes back to its ViewPager2 page, or a dialog over it is dismissed) now gets a new nav transaction for its current destination if its previous one has finished. Before, spans produced on return had no transaction to attach to until the next navigation. StagedNavTransaction keeps the destination's NavTransactionRequest after its transaction finishes or can't start, and starts a new transaction from it on the next publication. The request replaces the NavTransaction handle that recorded writes for replay, and is kept in step with back stack changes below the top via updateNavTransactionRequest. --- CHANGELOG.md | 4 + .../compose/navigation3/BackStackObserver.kt | 129 ++---- .../navigation3/lifecycle/NavScopes.kt | 65 ++- .../navigation3/lifecycle/NavTransaction.kt | 149 ------- .../lifecycle/NavTransactionRequest.kt | 103 +++++ .../compose/navigation3/lifecycle/README.md | 33 +- .../lifecycle/StagedNavTransaction.kt | 206 +++++----- .../navigation3/BackStackObserverTest.kt | 6 +- .../CoordinatedBackStackObserverTest.kt | 133 +++++- ...SentryNavEffectLifecycleIntegrationTest.kt | 110 ++++- .../navigation3/lifecycle/NavScopeTest.kt | 76 ++-- .../lifecycle/NavScopesIntegrationTest.kt | 8 +- .../navigation3/lifecycle/NavScopesTest.kt | 191 +++++++-- .../lifecycle/NavTransactionRequestTest.kt | 244 +++++++++++ .../lifecycle/NavTransactionTest.kt | 348 ---------------- .../lifecycle/StagedNavTransactionTest.kt | 385 ++++++++++++------ 16 files changed, 1224 insertions(+), 966 deletions(-) delete mode 100644 sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt create mode 100644 sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionRequest.kt create mode 100644 sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionRequestTest.kt delete mode 100644 sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index d3243f9991b..f46da095067 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,10 @@ - [changelog](https://github.com/square/okhttp/blob/master/CHANGELOG.md) - [diff](https://github.com/square/okhttp/compare/parent-4.9.2...parent-5.5.1) +### Behavioral Changes + +- Nav3: Returning to an Activity or Fragment with its own `SentryNavEffect` within the app (e.g., closing the Activity that covered it, swiping back to its ViewPager2 page, or dismissing a dialog over it) now starts a new navigation transaction for the current destination if the previous one finished, so spans produced upon return can now be consistently captured ([#6482](https://github.com/getsentry/sentry-java/pull/6482)) + ## 8.63.0 ### Features diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt index 0becec8b705..d3e92777ce3 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt @@ -5,23 +5,18 @@ import io.sentry.Hint import io.sentry.SentryIntegrationPackageStorage import io.sentry.SentryLevel.INFO import io.sentry.SentryOptions -import io.sentry.TransactionContext -import io.sentry.TransactionOptions import io.sentry.compose.navigation3.BackStackConverter.RetentionPolicy import io.sentry.compose.navigation3.PreparedChange.BackStackHasNewTop import io.sentry.compose.navigation3.PreparedChange.BackStackHasSameTop import io.sentry.compose.navigation3.PreparedChange.BackStackIsEmpty import io.sentry.compose.navigation3.lifecycle.INavScope import io.sentry.compose.navigation3.lifecycle.INavScopes -import io.sentry.compose.navigation3.lifecycle.INavTransaction import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY -import io.sentry.protocol.App -import io.sentry.protocol.TransactionNameSource +import io.sentry.compose.navigation3.lifecycle.NAVIGATION_OP +import io.sentry.compose.navigation3.lifecycle.NavTransactionRequest import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion import java.lang.ref.WeakReference -private const val NAVIGATION_OP: String = "navigation" - /** * Observes the back stack managed by a single [SentryNavEffect] and records Sentry state as the * back stack is updated. @@ -57,10 +52,9 @@ internal class BackStackObserver( private val backStackConverter = BackStackConverter(entryMapper, scopes.options.logger) - private val navTransactions = NavTransactions(scopes) private val navBreadcrumbs = NavBreadcrumbs(scopes) private val navScreen = NavScreen() - private val navContext = NavContext(scopes, options) + private val navContext = NavContext(options) // Safe because the host back stack retains the current top entry strongly between updates. private var previousTop: WeakReference? = null @@ -192,16 +186,10 @@ internal class BackStackObserver( navBreadcrumbs.emit(fromEntry = previousTop, toEntry = currentBackStack.topEntryNormalized) } - navTransactions.stop(scope) + scope.finishNavTransaction() if (areNavigationTransactionsEnabled) { - navTransactions - .start( - scope, - currentTop.name, - currentTop.argumentsWithMetadata(), - ) - ?.let { transaction -> navContext.updateTransaction(transaction, scope, currentBackStack) } + scope.startNavTransaction(navTransactionRequestFor(currentBackStack)) } else { scope.rotatePropagationContext() } @@ -211,6 +199,13 @@ internal class BackStackObserver( private fun handleSameTop(scope: INavScope, backStack: BackStackData) { navContext.update(scope, backStack.capturedEntriesNormalized) + + // Keeps the nav context of transactions started later for the same destination (e.g., when the + // user returns to the nav root) in step with the back stack. + if (areNavigationTransactionsEnabled) { + scope.updateNavTransactionRequest(navTransactionRequestFor(backStack)) + } + storePreviousTop(backStack) } @@ -228,8 +223,20 @@ internal class BackStackObserver( previousTopNormalized = null } + private fun navTransactionRequestFor(backStack: BackStackData): NavTransactionRequest { + val top = backStack.topEntryNormalized + + return NavTransactionRequest.forDestination( + name = top.name, + arguments = top.argumentsWithMetadata(), + contexts = navContext.forTransaction(backStack.capturedEntriesNormalized), + viewNames = if (scopes.options.isEnableScreenTracking) listOf(top.name) else null, + options = scopes.options, + ) + } + private fun tearDown(scope: INavScope) { - navTransactions.stop(scope) + scope.finishNavTransaction() navContext.clear(scope) navScreen.clear(scope) clearPreviousTop() @@ -274,58 +281,8 @@ private data class BackStackData( val capturedEntriesNormalized: List, ) -/** A helper class for managing nav transactions. */ -private class NavTransactions(private val scopes: INavScopes) { - - private companion object { - private const val TRANSACTION_ORIGIN = "auto.navigation.nav3" - } - - /** - * Starts an idle navigation transaction, or no-ops if another transaction is already active. - * - * [Stop][stop] the current navigation transaction first. See [INavScope.startNavTransaction]. - */ - fun start( - scope: INavScope, - name: String, - arguments: Map, - ): INavTransaction? { - val transactionOptions = - TransactionOptions().also { - it.isWaitForChildren = true - it.idleTimeout = scopes.options.idleTimeout - val deadlineTimeoutMillis = scopes.options.deadlineTimeout - it.deadlineTimeout = if (deadlineTimeoutMillis <= 0) null else deadlineTimeoutMillis - it.isTrimEnd = true - it.origin = TRANSACTION_ORIGIN - } - - val transaction = - scope.startNavTransaction( - TransactionContext(name, TransactionNameSource.ROUTE, NAVIGATION_OP), - transactionOptions, - ) ?: return null - - if (arguments.isNotEmpty()) { - transaction.setData("arguments", arguments) - } - - return transaction - } - - /** - * Finishes and unsets the active navigation transaction, if one exists. - * - * See [INavScope.finishNavTransaction]. - */ - fun stop(scope: INavScope) { - scope.finishNavTransaction() - } -} - /** A helper class for updating [nav context][NAVIGATION_CONTEXT_KEY]. */ -private class NavContext(private val scopes: INavScopes, private val options: SentryNavOptions) { +private class NavContext(private val options: SentryNavOptions) { private companion object { private const val BACKSTACK_KEY = "Back Stack" @@ -345,33 +302,21 @@ private class NavContext(private val scopes: INavScopes, private val options: Se } /** - * Updates the transaction with the provided navigation info. + * The nav context to set on the nav transaction for a destination whose back stack is + * [capturedEntries], if any. * * Needed because transactions inherit base scope context on a per-key basis unless transactions - * have their own values for those keys. In our case, we need to keep fresh back stack and route - * values in the base context for purposes of crash reporting. But those values will often advance - * past what's relevant to a given transaction. This method prevents misassociation by binding - * proper values to the transaction context instead. + * have their own values for those keys. In our case, we need to keep fresh back stack values in + * the base context for purposes of crash reporting. But those values will often advance past + * what's relevant to a given transaction. Setting the destination's own back stack on its + * transaction prevents misassociation. */ - fun updateTransaction( - transaction: INavTransaction, - scope: INavScope, - backStack: BackStackData, - ) { - if (scopes.options.isEnableScreenTracking) { - val appContext = scope.app ?: App() - - appContext.viewNames = listOf(backStack.topEntryNormalized.name) - transaction.setApp(appContext) - } - - if (options.captureBackStack && backStack.capturedEntriesNormalized.isNotEmpty()) { - transaction.setContext( - NAVIGATION_CONTEXT_KEY, - backStack.capturedEntriesNormalized.toBackStackMap(), - ) + fun forTransaction(capturedEntries: List): Map = + if (options.captureBackStack && capturedEntries.isNotEmpty()) { + mapOf(NAVIGATION_CONTEXT_KEY to capturedEntries.toBackStackMap()) + } else { + emptyMap() } - } /** Builds the `{"backstack": [...]}` map bound under [NAVIGATION_CONTEXT_KEY]. */ private fun List.toBackStackMap(): Map = diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt index 8c1c6d32b7e..39568fa2a29 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt @@ -8,10 +8,6 @@ import io.sentry.IScopes import io.sentry.PropagationContext import io.sentry.SentryLevel import io.sentry.SentryOptions -import io.sentry.TransactionContext -import io.sentry.TransactionOptions -import io.sentry.protocol.App -import io.sentry.protocol.Contexts /** The key under which nav context (i.e., the captured back stack) is stored in a scope. */ internal const val NAVIGATION_CONTEXT_KEY: String = "Navigation" @@ -194,15 +190,6 @@ internal class NavScopes( } } - /** - * Copies the whole of [IScope.getContexts] in order to return a copy of its [App]. - * - * [App]'s copy constructor isn't public, so an `App` can't be copied on its own. Going through - * [Contexts] deep-copies every context on the scope and discards all of it but the `App`. - */ - override val app: App? - get() = Contexts(delegate.contexts).app - private val hasLease: Boolean get() = registration?.hasLease == true @@ -221,6 +208,10 @@ internal class NavScopes( } } + override fun updateNavTransactionRequest(request: NavTransactionRequest) { + stagedNavTransaction.updateRequest(request) + } + /** * Starts and binds a nav transaction with the nav lease, or defers it until the lease is * acquired (see [StagedNavTransaction]). Callers must [finish][finishNavTransaction] the @@ -234,15 +225,12 @@ internal class NavScopes( * meantime attach to whichever transaction is bound to the scope at that time, usually the * leaseholder's. */ - override fun startNavTransaction( - transactionContext: TransactionContext, - transactionOptions: TransactionOptions, - ): INavTransaction? { - return if (hasLease) { - stagedNavTransaction.start(delegate, transactionContext, transactionOptions) + override fun startNavTransaction(request: NavTransactionRequest) { + stagedNavTransaction.stage(request) + if (hasLease) { + stagedNavTransaction.publishTo(delegate) } else { stagedNavData.deferPropagationContextRotation() - stagedNavTransaction.stage(transactionContext, transactionOptions) } } @@ -286,17 +274,6 @@ internal sealed interface INavScope { var screen: String? - /** - * A copy of the scope's app context, if any. - * - * A copy rather than the live instance, because callers mutate the result before binding it to a - * transaction, and mutating the scope's own [App] would corrupt shared scope state. - * - * Implementations may have to copy more than the [App] to produce it, so treat this as expensive: - * read it once where it's needed rather than per frame or in a loop. - */ - val app: App? - /** * Sets or, given `null`, clears the value stored under [NAVIGATION_CONTEXT_KEY]. * @@ -309,21 +286,29 @@ internal sealed interface INavScope { fun rotatePropagationContext() /** - * Starts a nav transaction and binds it to the scope, unless another unfinished transaction is - * already bound to it. Returns `null` if no transaction was started. + * Replaces the request kept from the most recent [startNavTransaction] with [request] (e.g., to + * reflect a back stack whose entries below the top changed), so that transactions started for the + * same destination later are started from it. A transaction that has already started keeps what + * it was started with. + * + * Does nothing if no request is kept, i.e., none was made since the last [finishNavTransaction]. + */ + fun updateNavTransactionRequest(request: NavTransactionRequest) + + /** + * Starts a nav transaction for [request] and binds it to the scope, unless another unfinished + * transaction is already bound to it, in which case none is started. * * (Note: Callers must first [finish][finishNavTransaction] the nav transaction most recently * requested, if any. Otherwise, with the nav lease, it blocks the new one while it's bound; * without the lease, it's replaced without being finished.) * - * The start may be deferred until the nav lease is acquired, in which case writes to the returned - * transaction are staged in the meantime, and whether another transaction blocks it is decided - * then (see [StagedNavTransaction.publishTo]). + * The start may be deferred until the nav lease is acquired, in which case whether another + * transaction blocks it is decided then (see [StagedNavTransaction.publishTo]). Either way, the + * request is kept until the next [finishNavTransaction], so that a new transaction can be started + * from it if the user returns to the nav root after this one finished. */ - fun startNavTransaction( - transactionContext: TransactionContext, - transactionOptions: TransactionOptions, - ): INavTransaction? + fun startNavTransaction(request: NavTransactionRequest) /** * Finishes the nav transaction most recently requested via [startNavTransaction] and unbinds it diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt deleted file mode 100644 index c8c1464687c..00000000000 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt +++ /dev/null @@ -1,149 +0,0 @@ -package io.sentry.compose.navigation3.lifecycle - -import io.sentry.IScopes -import io.sentry.ITransaction -import io.sentry.SpanStatus -import io.sentry.TransactionContext -import io.sentry.TransactionOptions -import io.sentry.protocol.App - -/** - * An [INavTransaction] handle backed by an [ITransaction] that's either started right away or, when - * the nav lease isn't held, [started later][start]. - * - * A deferred handle records what's set on it and replays it onto the transaction once started. - * [Finishing][finish] a deferred handle before it starts discards it, so its transaction never - * starts. - * - * **Thread safety** - * - * This class is ***not*** thread-safe. Apply the thread safety policy from [NavScopes]. - */ -internal class NavTransaction -private constructor( - val name: String, - private var state: State, -) : INavTransaction { - - /** - * The backing transaction, once [started][State.Started]; `null` while [pending][State.Pending] - * or once [discarded][State.Discarded]. - */ - val transaction: ITransaction? - get() = (state as? State.Started)?.transaction - - override fun setData(key: String, value: Any) { - when (val state = state) { - is State.Pending -> state.data[key] = value - is State.Started -> state.transaction.setData(key, value) - State.Discarded -> Unit - } - } - - override fun setContext(key: String, value: Any) { - when (val state = state) { - is State.Pending -> state.contexts[key] = value - is State.Started -> state.transaction.setContext(key, value) - State.Discarded -> Unit - } - } - - override fun setApp(app: App) { - when (val state = state) { - is State.Pending -> state.app = app - is State.Started -> state.transaction.contexts.setApp(app) - State.Discarded -> Unit - } - } - - /** - * Starts a deferred handle's transaction via [scopes] and replays what was set on the handle onto - * it. - * - * Returns the started transaction, or `null` if the handle was started already or discarded, or - * [scopes] declined to start one. - */ - fun start(scopes: IScopes): ITransaction? { - val pending = state as? State.Pending ?: return null - val transaction = - scopes.startTransaction(pending.context, pending.options).takeUnless { it.isNoOp } - ?: return null - - state = State.Started(transaction) - pending.data.forEach { (key, value) -> transaction.setData(key, value) } - pending.contexts.forEach { (key, value) -> transaction.setContext(key, value) } - pending.app?.let { transaction.contexts.setApp(it) } - return transaction - } - - /** - * Finishes the backing transaction with its own status, or [OK][SpanStatus.OK] if it has none, - * and then passes it to [onFinished]. - * - * A deferred handle that hasn't started is discarded instead, so it never starts, and - * [onFinished] isn't called. - */ - fun finish(onFinished: (ITransaction) -> Unit) { - when (val state = state) { - is State.Pending -> this.state = State.Discarded - is State.Started -> { - if (!state.transaction.isFinished) { - state.transaction.finish(state.transaction.status ?: SpanStatus.OK) - } - onFinished(state.transaction) - } - State.Discarded -> Unit - } - } - - /** Lifecycle state for a [NavTransaction] handle. */ - private sealed interface State { - - /** - * Not started yet: the transaction will be started from [context] and [options], and what's set - * on the handle until then is recorded for replay. - */ - class Pending(val context: TransactionContext, val options: TransactionOptions) : State { - val data = linkedMapOf() - val contexts = linkedMapOf() - var app: App? = null - } - - /** The backing [transaction] has been started. A terminal [State]. */ - class Started(val transaction: ITransaction) : State - - /** The handle was finished before its backing transaction was started. A terminal [State]. */ - data object Discarded : State - } - - companion object { - /** A handle backed by the already-started [transaction]. */ - fun started(transaction: ITransaction): NavTransaction = - NavTransaction(transaction.name, State.Started(transaction)) - - /** A handle whose transaction is started later, via [start]. */ - fun deferred(context: TransactionContext, options: TransactionOptions): NavTransaction = - NavTransaction(context.name, State.Pending(context, options)) - } -} - -/** - * The subset of the [ITransaction] interface used to record nav data on a nav transaction. - * - * Protects against accidental misuse of what would otherwise be a partially-implemented - * `ITransaction` interface. New operations against the underlying `ITransaction` instance must be - * added here. Operations not directly delegable to `ITransaction` should ***not*** be added. - * - * Starting and finishing a nav transaction are lease-aware, so they go through [INavScope] instead. - */ -internal sealed interface INavTransaction { - - /** See [ITransaction.setData]. */ - fun setData(key: String, value: Any) - - /** See [ITransaction.setContext]. */ - fun setContext(key: String, value: Any) - - /** Sets the transaction's app context. See [ITransaction.getContexts]. */ - fun setApp(app: App) -} diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionRequest.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionRequest.kt new file mode 100644 index 00000000000..343518b6542 --- /dev/null +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionRequest.kt @@ -0,0 +1,103 @@ +package io.sentry.compose.navigation3.lifecycle + +import io.sentry.IScope +import io.sentry.IScopes +import io.sentry.ITransaction +import io.sentry.SentryOptions +import io.sentry.TransactionContext +import io.sentry.TransactionOptions +import io.sentry.protocol.App +import io.sentry.protocol.Contexts +import io.sentry.protocol.TransactionNameSource + +/** The operation of nav transactions, and the type and category of nav breadcrumbs. */ +internal const val NAVIGATION_OP: String = "navigation" + +/** + * Everything needed to start a nav transaction for a destination. + * + * Transactions started from a request are named [name] with [TransactionNameSource.ROUTE] as their + * name source, and have the operation [NAVIGATION_OP]. + * + * A request may be reused to start a new transaction if the user returns to a nav root after its + * transaction finishes. Each [start] creates a new transaction for the same destination. + */ +internal class NavTransactionRequest( + val name: String, + /** Builds the options for each transaction started from this request. */ + private val newOptions: () -> TransactionOptions, + /** Data set on each transaction started from this request. See [ITransaction.setData]. */ + private val data: Map = emptyMap(), + /** Contexts set on each transaction started from this request. See [ITransaction.setContext]. */ + private val contexts: Map = emptyMap(), + /** + * The view names to set on the app context of each transaction started from this request, or + * `null` to leave the transaction's app context alone. The rest of the app context is copied from + * the scope when the transaction starts. + */ + private val viewNames: List? = null, +) { + + companion object { + + private const val TRANSACTION_ORIGIN = "auto.navigation.nav3" + + /** Creates a request whose transaction options are read when each transaction starts. */ + fun forDestination( + name: String, + arguments: Map, + contexts: Map, + viewNames: List?, + options: SentryOptions, + ): NavTransactionRequest = + NavTransactionRequest( + name = name, + newOptions = { + TransactionOptions().also { + it.isWaitForChildren = true + it.idleTimeout = options.idleTimeout + val deadlineTimeoutMillis = options.deadlineTimeout + it.deadlineTimeout = if (deadlineTimeoutMillis <= 0) null else deadlineTimeoutMillis + it.isTrimEnd = true + it.origin = TRANSACTION_ORIGIN + } + }, + data = if (arguments.isNotEmpty()) mapOf("arguments" to arguments) else emptyMap(), + contexts = contexts, + viewNames = viewNames, + ) + } + + /** + * Starts a transaction via [scopes] with this request's data and contexts. Copies the app context + * from [publishingScope] when setting view names, without binding the transaction to that scope. + * + * Returns `null` if [scopes] declined to start one (i.e., returned a no-op transaction, e.g., + * because the SDK is disabled or ignores the transaction's origin). + */ + fun start(scopes: IScopes, publishingScope: IScope): ITransaction? { + val transaction = + scopes + .startTransaction( + TransactionContext(name, TransactionNameSource.ROUTE, NAVIGATION_OP), + newOptions(), + ) + .takeUnless { it.isNoOp } ?: return null + + data.forEach { (key, value) -> transaction.setData(key, value) } + contexts.forEach { (key, value) -> transaction.setContext(key, value) } + viewNames?.let { + transaction.contexts.setApp(publishingScope.appCopy().apply { this.viewNames = it }) + } + return transaction + } + + /** + * A copy of the receiver's app context, or a new one if it has none, so that setting view names + * on it leaves the scope's own app context alone. + * + * [App]'s copy constructor isn't public, so this copies all of [IScope.getContexts] and discards + * all of it but the `App`. + */ + private fun IScope.appCopy(): App = Contexts(contexts).app ?: App() +} diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md index d3e1a942bdf..c6aa677e7ce 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md @@ -53,7 +53,8 @@ The lease is acquired preemptively by an incoming nav root's `NavScopes` instanc A displaced nav root remains eligible for lease restoration until it stops (`ON_STOP`). It gets the lease back when it resumes (`ON_RESUME`), or sooner if the new nav root leaves or pauses while the displaced nav root is still visible. If restored, the displaced nav -root's nav data and unfinished transaction are reinstated. +root's nav data is reinstated, and so is its nav transaction if unfinished. Otherwise, the nav root gets a new nav transaction (see +[below](#returning-to-a-nav-root)). > † The exception is an incoming nav root that isn't resumed while the leaseholder is, e.g., a preloaded ViewPager2 Fragment page next to the current one. The user isn't interacting with such a @@ -138,15 +139,31 @@ Alternative at rollback point: B disposes before A stops Unbind txB ◄─────────────── dispose B ───┤ Navigation lease: A │ Republish A's staged data │ - Restore txA if unfinished and scope free │ + Restore txA if unfinished, else start a │ + new txA; bind it if the scope is free │ Finish txB if unfinished │ ``` -### Nav transactions follow navigation, not the lease +### Returning to a nav root -A nav root gets a nav transaction for a destination it navigates to, never merely for acquiring the lease. A destination reached without -the lease gets its transaction when the lease is next acquired, if it can start then; otherwise it gets none. On reacquiring the lease -without having navigated in the meantime, a nav root only gets its unfinished transaction back, if any. +A nav root is "returned to" when its candidate acquires the lease at any time other than on its first back stack update, e.g., when the +Activity covering it closes, or when the user swipes back to its ViewPager2 page. Spans produced on return (e.g., by work the nav root +resumes) need a nav transaction to attach to: -Hence the asymmetry for, e.g., ViewPager2 pages: the first time the user swipes to a preloaded page, it gets the transaction for the -destination it reached while offscreen, but swiping back to a page whose transaction has finished starts none. +- if the candidate's nav transaction is still active, that transaction is bound again, which + keeps the spans with the transaction that already holds the destination's earlier spans; and + +- if the candidate's nav transaction is finished (e.g., the transaction timed out while another + nav root held the lease, or it never started because another transaction held the scope's + transaction slot), a new nav transaction is started for the current destination. + +Either way, the transaction is bound only if the scope's transaction slot is free (see `StagedNavTransaction`). Rebinding and starting both +happen when the candidate publishes (`INavStatePublisher.publishTo`) its nav state, so the coordinator needn't tell a return from a first +acquisition: on its first back stack update, a candidate has no destination to start a transaction for yet. + +A nav root that's destroyed and recreated after losing the lease (e.g., on a configuration change) isn't returned to: it registers anew and +gets a transaction on its first back stack update. See [#6454](https://github.com/getsentry/sentry-java/issues/6454). + +Bringing the app back to the foreground isn't a return either: the leaseholder keeps the lease while the app is in the background (see +`stopCandidate`), so it starts no new nav transaction when it resumes. This matches the rest of the SDK, which starts no transaction when an +existing Activity comes back to the foreground. diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt index 2851a10481c..59916420c76 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt @@ -4,20 +4,29 @@ import io.sentry.IScope import io.sentry.IScopes import io.sentry.ITransaction import io.sentry.SentryLevel -import io.sentry.TransactionContext -import io.sentry.TransactionOptions +import io.sentry.SpanStatus /** * Holds a nav transaction staged for publication to a shared [IScope]. * - * A transaction requested while publishing isn't permitted is [staged][stage] rather than started, - * and starts when it's next [published][publishTo]. That publication is its only chance to start: - * if it can't start then, it's discarded. A started transaction that's [unpublished][unpublishFrom] - * is unbound but keeps running, so that it can be bound again if it's republished while unfinished. - * Once [finished][finish], a transaction is forgotten and never bound again. + * A [staged][stage] request starts a transaction when it's next [published][publishTo], which + * happens right away if the caller holds the nav lease. An [unpublished][unpublishFrom] transaction + * keeps running and can be rebound. [finish] forgets both the transaction and its request. * - * Starting or publishing a nav transaction never displaces an unfinished transaction bound to the - * scope by someone else (e.g., the host app). + * If a transaction ends on its own, cannot start, or is discarded, its request remains available to + * start a new transaction when the nav root returns. + * + * **Sampling** + * + * Sampled and unsampled transactions behave alike while running. Returning to a nav root reuses its + * unfinished transaction and sampling decision; starting a new transaction on return makes a new + * sampling decision. + * + * **Precedence** + * + * Navigation doesn't displace an unfinished transaction already bound to the shared `IScope`. This + * includes `ui.load`, `ui.action`, and host-app transactions. Changing their relative priority + * requires an SDK-wide policy. * * **Thread safety** * @@ -26,130 +35,112 @@ import io.sentry.TransactionOptions internal class StagedNavTransaction(private val scopes: IScopes) { /** - * The nav transaction most recently started or staged, unless it has been finished since then. + * The request most recently passed to [stage], whatever became of its transaction. Cleared when + * the nav transaction is [finished][finish] or [forgotten][forget], i.e., when the destination it + * describes is left. + * + * While there's a request but no [transaction], a transaction is started from the request when + * it's next [published][publishTo]: it's staged, or was discarded or blocked from starting. */ - private var navTransaction: NavTransaction? = null + private var request: NavTransactionRequest? = null + + /** The transaction most recently started for [request], unless it has been forgotten since. */ + private var transaction: ITransaction? = null /** - * Starts a nav transaction from [context] and [options] and binds it to [scope], unless another - * unfinished transaction is already bound to `scope`, in which case none is started, as it - * couldn't be bound. Returns `null` if no transaction was started (including when [scopes] - * returned a no-op transaction, e.g., because the SDK is disabled or ignores the transaction's - * origin), in which case the instance's current nav transaction, if any, is kept. + * Stages [request], so that the next [publishTo] starts a transaction for it. * - * **Warning**: [Finish][finish] the current nav transaction before calling this method, as this - * method replaces the current transaction without finishing it. + * Call [finish] first: staging replaces the current transaction without finishing it. */ - fun start( - scope: IScope, - context: TransactionContext, - options: TransactionOptions, - ): NavTransaction? { - if (!scope.isFreeFor(context.name, transaction = null)) { - return null - } - val transaction = - scopes.startTransaction(context, options).takeUnless { it.isNoOp } ?: return null - - // Record the transaction before binding it: binding notifies the host app's scope observers, - // which can throw, and an unrecorded transaction might never be finished. - val started = NavTransaction.started(transaction) - navTransaction = started - - bind(scope, transaction) - return started + fun stage(request: NavTransactionRequest) { + this.request = request + transaction = null } /** - * Stages a nav transaction that's started from [context] and [options] when it's - * [published][publishTo], and returns its handle, which records what's set on it in the meantime. + * Updates the saved request for a future start without changing a running transaction. * - * **Warning**: [Finish][finish] the current nav transaction before calling this method, as this - * method replaces the current transaction without finishing it. + * No-ops if no request has been [staged][stage]. */ - fun stage(context: TransactionContext, options: TransactionOptions): NavTransaction = - NavTransaction.deferred(context, options).also { navTransaction = it } + fun updateRequest(request: NavTransactionRequest) { + if (this.request == null) { + return + } + this.request = request + } /** - * Binds the nav transaction to [scope], starting it first if it's staged. A started transaction - * that has finished in the meantime is forgotten and unbound instead. - * - * If another unfinished transaction is bound to [scope], a started transaction stays unbound, so - * it can be bound the next time it's published. A staged transaction is discarded instead, as it - * is if [scopes] return a no-op transaction for it (see [start]). + * Publishes the staged transaction by binding it to [scope], or starts a transaction from the + * staged request if needed. An occupied scope slot leaves the transaction unbound and the request + * available for later publication. */ fun publishTo(scope: IScope) { - val navTransaction = navTransaction ?: return - val started = navTransaction.transaction - when { - started?.isFinished == true -> { - this.navTransaction = null - unbind(scope, started) - } - started != null -> if (scope.isFreeFor(navTransaction.name, started)) bind(scope, started) - else -> { - val startedNow = - if (scope.isFreeFor(navTransaction.name, transaction = null)) { - navTransaction.start(scopes) - } else { - null - } - if (startedNow != null) { - bind(scope, startedNow) - } else { - this.navTransaction = null - navTransaction.finish(onFinished = {}) - } + val request = request ?: return + val started = transaction + + if (started != null && !started.isFinished) { + if (scope.isFreeFor(request.name, started)) { + bind(scope, started) } + return } + + transaction = null + started?.let { unbind(scope, it) } + startAndBind(scope, request) } - /** - * Unbinds the nav transaction from [scope], if it's bound, without finishing it, so it can be - * [published][publishTo] again. - */ + /** Unbinds the transaction from [scope] without finishing it, allowing later publication. */ fun unpublishFrom(scope: IScope) { - navTransaction?.transaction?.let { unbind(scope, it) } + transaction?.let { unbind(scope, it) } } /** - * Finishes the nav transaction and unbinds it from [scope], if it's bound. A staged transaction - * is discarded instead. + * Finishes and unbinds the transaction from [scope], or discards it if staged. Uses its current + * status, or [OK][SpanStatus.OK] if unset. Forgets the request in either case. */ fun finish(scope: IScope) { - val navTransaction = navTransaction ?: return + request = null + val transaction = transaction ?: return // Forget the transaction before unbinding it: unbinding notifies the host app's scope // observers, which can throw, and a transaction left behind would be bound again later. - this.navTransaction = null - navTransaction.finish(onFinished = { unbind(scope, it) }) + this.transaction = null + if (!transaction.isFinished) { + transaction.finish(transaction.status ?: SpanStatus.OK) + } + unbind(scope, transaction) } - /** - * Forgets the nav transaction without finishing it, e.g., when there's no scope to unbind it - * from. If started, it will be left to time out. - */ + /** Forgets the transaction and request without finishing or unbinding a started transaction. */ fun forget() { - navTransaction = null + request = null + transaction = null } + /** Whether the slot is empty, holds a finished transaction, or already holds [transaction]. */ + private fun canHold(current: ITransaction?, transaction: ITransaction?): Boolean = + current == null || current.isFinished || current === transaction + /** - * Reports whether the receiver's transaction slot can hold [transaction], or a new transaction - * given `null` (see [canHold]). + * Starts and binds a transaction based on the provided [request] if [scope]'s slot is free. + * + * Returns `null` if the slot is occupied or [scopes] declines the start. */ - private fun IScope.isFreeFor(name: String, transaction: ITransaction?): Boolean { - if (canHold(current = this.transaction, transaction)) { - return true + private fun startAndBind(scope: IScope, request: NavTransactionRequest): ITransaction? { + if (!scope.isFreeFor(request.name, transaction = null)) { + return null } + val transaction = request.start(scopes, publishingScope = scope) ?: return null - scopes.options.logger.log( - SentryLevel.DEBUG, - "Nav3 transaction for route %s is blocked by another active transaction.", - name, - ) - return false + // Record the transaction before binding it: binding notifies the host app's scope observers, + // which can throw, and an unrecorded transaction might never be finished. + this.transaction = transaction + + bind(scope, transaction) + return transaction } - /** Binds [transaction] to [scope] if the scope's transaction slot can hold it (see [canHold]). */ + /** Binds [transaction] if [scope]'s slot is empty, finished, or already holds it. */ private fun bind(scope: IScope, transaction: ITransaction) { scope.withTransaction { current -> if (current !== transaction && canHold(current, transaction)) { @@ -158,13 +149,6 @@ internal class StagedNavTransaction(private val scopes: IScopes) { } } - /** - * Whether a transaction slot holding [current] can hold [transaction], or a new transaction given - * `null`: it's empty, holds a finished transaction, or already holds `transaction`. - */ - private fun canHold(current: ITransaction?, transaction: ITransaction?): Boolean = - current == null || current.isFinished || current === transaction - /** Unbinds [transaction] from [scope] if it's the bound transaction. */ private fun unbind(scope: IScope, transaction: ITransaction) { scope.withTransaction { current -> @@ -173,4 +157,20 @@ internal class StagedNavTransaction(private val scopes: IScopes) { } } } + + /** + * Checks whether this scope can hold [transaction] (or a new one if `null`), logging a blocker. + */ + private fun IScope.isFreeFor(name: String, transaction: ITransaction?): Boolean { + if (canHold(current = this.transaction, transaction)) { + return true + } + + scopes.options.logger.log( + SentryLevel.DEBUG, + "Nav3 transaction for route %s is blocked by another active transaction.", + name, + ) + return false + } } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackObserverTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackObserverTest.kt index ef79931174f..d7d8895c608 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackObserverTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackObserverTest.kt @@ -63,6 +63,9 @@ class BackStackObserverTest { val breadcrumbHints = mutableListOf() val startedTransactions = mutableListOf() + /** The [NavScopes] passed to the observer created by [getSut]. */ + lateinit var navScopes: NavScopes + init { whenever(scopes.options).thenReturn(scope.options) whenever(scopes.getSpan()).thenAnswer { scope.span } @@ -95,9 +98,10 @@ class BackStackObserverTest { entryMapper: BackStackEntryMapper = defaultEntryMapper, ): BackStackObserver { scope.options.isEnableScreenTracking = config.enableScreenTracking + navScopes = leaseholderNavScopes() return BackStackObserver( - scopes = leaseholderNavScopes(), + scopes = navScopes, options = SentryNavOptions { enableNavigationBreadcrumbs = config.enableNavigationBreadcrumbs diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt index dea07db5481..6164f479bab 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt @@ -13,12 +13,14 @@ import io.sentry.Scope import io.sentry.ScopeCallback import io.sentry.SentryOptions import io.sentry.SentryTracer +import io.sentry.SpanContext import io.sentry.TransactionContext import io.sentry.TransactionOptions import io.sentry.compose.navigation3.lifecycle.INavStatePublisher import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator import io.sentry.compose.navigation3.lifecycle.NavRootInfo +import io.sentry.protocol.TransactionNameSource import org.junit.Test import org.mockito.kotlin.any import org.mockito.kotlin.doAnswer @@ -136,7 +138,7 @@ class CoordinatedBackStackObserverTest { val pageA = fixture.getSut(pageALifecycle) val pageB = fixture.getSut(pageBLifecycle) // E.g., the Activity's ui.load transaction, still running while the pager's pages load. - val hostTransaction = mock() + val hostTransaction = hostTransaction() fixture.scope.transaction = hostTransaction pageA.onBackStackChanged(listOf("PageA")) pageB.onBackStackChanged(listOf("PageB")) @@ -150,18 +152,19 @@ class CoordinatedBackStackObserverTest { } @Test - fun `a page swiped to while a host transaction is bound starts no nav transaction when swiped to again`() { + fun `pages whose nav transactions a host transaction blocked start them when swiped to after it finishes`() { val fixture = Fixture() val pageALifecycle = fixture.resumedLifecycle() val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } val pageA = fixture.getSut(pageALifecycle) val pageB = fixture.getSut(pageBLifecycle) - val hostTransaction = mock() + val hostTransaction = hostTransaction() fixture.scope.transaction = hostTransaction pageA.onBackStackChanged(listOf("PageA")) pageB.onBackStackChanged(listOf("PageB")) pageBLifecycle.currentState = Lifecycle.State.RESUMED pageALifecycle.currentState = Lifecycle.State.STARTED + check(fixture.startedTransactions.isEmpty()) whenever(hostTransaction.isFinished).thenReturn(true) // Swipe back to page A, then to page B again. @@ -170,9 +173,12 @@ class CoordinatedBackStackObserverTest { pageBLifecycle.currentState = Lifecycle.State.RESUMED pageALifecycle.currentState = Lifecycle.State.STARTED - // As with page A, which navigated while the host transaction was bound, page B's destination - // gets no nav transaction: it couldn't start one when it first got the nav lease. - assertThat(fixture.startedTransactions).isEmpty() + // Neither page could start a nav transaction while the host transaction was bound, so each + // starts one when it's returned to. + assertThat(fixture.startedTransactions.map { it.name }) + .containsExactly("/PageA", "/PageB") + .inOrder() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/PageB")) assertThat(fixture.scope.screen).isEqualTo("/PageB") } @@ -221,7 +227,7 @@ class CoordinatedBackStackObserverTest { } @Test - fun `swiping back to a page whose nav transaction finished starts no nav transaction`() { + fun `swiping back to a page whose nav transaction finished starts a new nav transaction for it`() { val fixture = Fixture() val pageALifecycle = fixture.resumedLifecycle() val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } @@ -237,11 +243,118 @@ class CoordinatedBackStackObserverTest { pageALifecycle.currentState = Lifecycle.State.RESUMED pageBLifecycle.currentState = Lifecycle.State.STARTED + assertThat(fixture.startedTransactions.map { it.name }) + .containsExactly("/PageA", "/PageB", "/PageA") + .inOrder() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.last()) + assertThat(fixture.scope.screen).isEqualTo("/PageA") + assertThat(fixture.breadcrumbs.map { it.data["to"] }) + .containsExactly("/PageA", "/PageB") + .inOrder() + } + + @Test + fun `the new nav transaction started when swiping back to a page reflects the page's nav data`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle, arguments = { mapOf("page" to it) }) + val pageB = fixture.getSut(pageBLifecycle) + pageA.onBackStackChanged(listOf("Home", "PageA")) + fixture.startedTransaction("/PageA").finish() + pageB.onBackStackChanged(listOf("PageB")) + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + + pageALifecycle.currentState = Lifecycle.State.RESUMED + pageBLifecycle.currentState = Lifecycle.State.STARTED + + val restarted = fixture.startedTransactions.last() + assertThat(restarted.name).isEqualTo("/PageA") + assertThat(restarted.transactionNameSource).isEqualTo(TransactionNameSource.ROUTE) + assertThat(restarted.operation).isEqualTo("navigation") + assertThat(restarted.spanContext.origin).isEqualTo("auto.navigation.nav3") + assertThat(restarted.getData("arguments")).isEqualTo(mapOf("page" to "PageA")) + assertThat(restarted.contexts.app?.viewNames).containsExactly("/PageA") + assertThat(restarted.contexts[NAVIGATION_CONTEXT_KEY]) + .isEqualTo( + mapOf( + "Back Stack" to + listOf( + mapOf("entry" to "/PageA", "entry_arguments" to mapOf("page" to "PageA")), + mapOf("entry" to "/Home", "entry_arguments" to mapOf("page" to "Home")), + ) + ) + ) + } + + @Test + fun `the new nav transaction started when swiping back to a page reflects its updated back stack`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle) + val pageATop = "PageA" + pageA.onBackStackChanged(listOf("Home", pageATop)) + fixture.startedTransaction("/PageA").finish() + pageB.onBackStackChanged(listOf("PageB")) + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + // Changes the entries below the top while page A is offscreen. + pageA.onBackStackChanged(listOf("Settings", pageATop)) + + pageALifecycle.currentState = Lifecycle.State.RESUMED + pageBLifecycle.currentState = Lifecycle.State.STARTED + + val restarted = fixture.startedTransactions.last() + assertThat(restarted.name).isEqualTo("/PageA") + assertThat(restarted.contexts[NAVIGATION_CONTEXT_KEY]) + .isEqualTo( + mapOf("Back Stack" to listOf(mapOf("entry" to "/PageA"), mapOf("entry" to "/Settings"))) + ) + } + + @Test + fun `swiping back to a page whose back stack became empty starts no nav transaction`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle) + pageA.onBackStackChanged(listOf("PageA")) + pageB.onBackStackChanged(listOf("PageB")) + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + pageA.onBackStackChanged(emptyList()) + + pageALifecycle.currentState = Lifecycle.State.RESUMED + pageBLifecycle.currentState = Lifecycle.State.STARTED + assertThat(fixture.startedTransactions.map { it.name }) .containsExactly("/PageA", "/PageB") .inOrder() assertThat(fixture.scope.transaction).isNull() - assertThat(fixture.scope.screen).isEqualTo("/PageA") + } + + @Test + fun `with nav transactions disabled, swiping back to a page starts no nav transaction`() { + val fixture = Fixture() + val options = SentryNavOptions { enableNavigationTransactions = false } + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle, options) + val pageB = fixture.getSut(pageBLifecycle, options) + pageA.onBackStackChanged(listOf("PageA")) + pageB.onBackStackChanged(listOf("PageB")) + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + + pageALifecycle.currentState = Lifecycle.State.RESUMED + pageBLifecycle.currentState = Lifecycle.State.STARTED + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() } @Test @@ -414,3 +527,7 @@ class CoordinatedBackStackObserverTest { override fun onUnregistered(scope: IScope?) = Unit } } + +/** A transaction the host app bound to the scope, as identified by its manual origin. */ +private fun hostTransaction(): ITransaction = + mock().also { whenever(it.spanContext).thenReturn(SpanContext("host")) } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt index d5467b10b9a..c30f296fd39 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt @@ -185,6 +185,36 @@ class SentryNavEffectLifecycleIntegrationTest { assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) } + @Test + fun `going back to an activity whose nav transaction finished while covered starts a new nav transaction for it`() { + val fixture = Fixture.create() + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + home.pause() + val checkout = Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + home.stop() + fixture.idle() + homeTransaction.finish() + + checkout.pause() + home.restart() + home.resume() + fixture.idle() + checkout.stop() + checkout.destroy() + fixture.idle() + + assertThat(fixture.scope.screen).isEqualTo("/Home") + assertThat(fixture.scope.transaction?.name).isEqualTo("/Home") + assertThat(fixture.scope.transaction).isNotSameInstanceAs(homeTransaction) + assertThat(fixture.scope.transaction?.isFinished).isFalse() + assertThat(fixture.scope.breadcrumbs.map { it.data["to"] }) + .containsExactly("/Home", "/Checkout") + .inOrder() + } + // Activity recreation @Test fun `a recreated activity publishes its own nav data and transaction over the destroyed instance's`() { @@ -440,7 +470,7 @@ class SentryNavEffectLifecycleIntegrationTest { } @Test - fun `switching back to a view pager page whose nav transaction finished starts no nav transaction`() { + fun `switching back to a view pager page whose nav transaction finished starts a new nav transaction for the page`() { val fixture = Fixture.create() val activity = Robolectric.buildActivity(ViewPagerActivity::class.java).setup() fixture.idle() @@ -452,8 +482,52 @@ class SentryNavEffectLifecycleIntegrationTest { activity.get().viewPager.setCurrentItem(0, false) fixture.idle() - assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.transaction?.name).isEqualTo("/Home") + assertThat(fixture.scope.transaction).isNotSameInstanceAs(homeTransaction) + assertThat(fixture.scope.transaction?.isFinished).isFalse() assertThat(fixture.scope.screen).isEqualTo("/Home") + assertThat(fixture.scope.breadcrumbs.map { it.data["to"] }) + .containsExactly("/Home", "/Checkout") + .inOrder() + } + + /** + * A transaction that isn't sampled is still started and bound, so a return while it's running + * binds it again rather than sampling a new one, just as for a sampled transaction. + */ + @Test + fun `switching back to a view pager page restores its unfinished unsampled nav transaction`() { + val fixture = Fixture.create(tracesSampleRate = 0.0) + val activity = Robolectric.buildActivity(ViewPagerActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + check(homeTransaction.isSampled == false) + activity.get().viewPager.setCurrentItem(1, false) + fixture.idle() + + activity.get().viewPager.setCurrentItem(0, false) + fixture.idle() + + assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) + } + + @Test + fun `switching back to a view pager page whose unsampled nav transaction finished samples a new one`() { + val fixture = Fixture.create(tracesSampleRate = 0.0) + val activity = Robolectric.buildActivity(ViewPagerActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + activity.get().viewPager.setCurrentItem(1, false) + fixture.idle() + homeTransaction.finish() + + activity.get().viewPager.setCurrentItem(0, false) + fixture.idle() + + val restarted = requireNotNull(fixture.scope.transaction) + assertThat(restarted).isNotSameInstanceAs(homeTransaction) + assertThat(restarted.name).isEqualTo("/Home") + assertThat(restarted.isSampled).isFalse() } // Dialogs @@ -539,6 +613,30 @@ class SentryNavEffectLifecycleIntegrationTest { assertThat(homeTransaction.isFinished).isFalse() } + @Test + fun `dismissing a dialog fragment starts a new nav transaction for the still-resumed activity if its own finished`() { + val fixture = Fixture.create() + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + val dialog = PickerDialogFragment() + dialog.show(home.get().supportFragmentManager, "picker") + fixture.idle() + val pickerTransaction = requireNotNull(fixture.scope.transaction) + homeTransaction.finish() + + dialog.dismiss() + fixture.idle() + + assertThat(pickerTransaction.isFinished).isTrue() + assertThat(fixture.scope.transaction?.name).isEqualTo("/Home") + assertThat(fixture.scope.transaction).isNotSameInstanceAs(homeTransaction) + assertThat(fixture.scope.transaction?.isFinished).isFalse() + assertThat(fixture.scope.breadcrumbs.map { it.data["to"] }) + .containsExactly("/Home", "/Picker") + .inOrder() + } + // Screens without an effect @Test @@ -758,13 +856,13 @@ class SentryNavEffectLifecycleIntegrationTest { } } - private class Fixture private constructor() { + private class Fixture private constructor(tracesSampleRate: Double) { val options = SentryOptions().apply { dsn = "http://key@localhost/1" defaultScopeType = ScopeType.CURRENT - setTracesSampleRate(1.0) + setTracesSampleRate(tracesSampleRate) isEnableScreenTracking = true idleTimeout = null deadlineTimeout = 0 @@ -820,7 +918,7 @@ class SentryNavEffectLifecycleIntegrationTest { fun require(): Fixture = checkNotNull(current) { "Fixture.create() was not called" } - fun create(): Fixture { + fun create(tracesSampleRate: Double = 1.0): Fixture { val application: Application = ApplicationProvider.getApplicationContext() val packageManager = shadowOf(application.packageManager) listOf( @@ -835,7 +933,7 @@ class SentryNavEffectLifecycleIntegrationTest { ComponentName(application.packageName, activityClass.name) ) } - return Fixture().also { current = it } + return Fixture(tracesSampleRate).also { current = it } } } } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt index ad2c7b9ae40..5f8b0ccd003 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt @@ -13,7 +13,6 @@ import io.sentry.SentryOptions import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.TransactionOptions -import io.sentry.protocol.App import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any @@ -76,29 +75,6 @@ class NavScopeTest { assertThat(fixture.scope.screen).isEqualTo("Profile") } - @Test - fun `getApp() - returns a copy of the scope's app context`() { - val fixture = Fixture.create() - val navScopes = fixture.navScopes - val hostApp = App().apply { appName = "Host" } - fixture.scope.contexts.setApp(hostApp) - - val app = navScopes.withNavScope { it.app } - - assertThat(app?.appName).isEqualTo("Host") - assertThat(app).isNotSameInstanceAs(hostApp) - } - - @Test - fun `getApp() - returns null when the scope has no app context`() { - val fixture = Fixture.create() - val navScopes = fixture.navScopes - - val app = navScopes.withNavScope { it.app } - - assertThat(app).isNull() - } - @Test fun `setNavigationContext() - publishes the navigation context with the nav lease`() { val fixture = Fixture.create(ownsLease = true) @@ -163,9 +139,8 @@ class NavScopeTest { val fixture = Fixture.create(ownsLease = true) val navScopes = fixture.navScopes - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } + navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(navTransaction).isNotNull() assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.single()) } @@ -176,9 +151,8 @@ class NavScopeTest { val hostTransaction = mock() fixture.scope.transaction = hostTransaction - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } + navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(navTransaction).isNull() assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } @@ -190,9 +164,8 @@ class NavScopeTest { fixture.scope.transaction = mock().also { whenever(it.isFinished).thenReturn(true) } - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } + navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(navTransaction).isNotNull() assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.single()) } @@ -203,9 +176,8 @@ class NavScopeTest { val hostTransaction = mock() fixture.scope.transaction = hostTransaction - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } + navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(navTransaction).isNotNull() assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } @@ -229,9 +201,8 @@ class NavScopeTest { val navScopes = fixture.navScopes val otherTransaction = fixture.startAnotherOwnersNavTransaction() - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } + navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(navTransaction).isNotNull() assertThat(fixture.startedTransactions).containsExactly(otherTransaction) assertThat(fixture.scope.transaction).isSameInstanceAs(otherTransaction) } @@ -243,9 +214,8 @@ class NavScopeTest { val finished = mock().also { whenever(it.isFinished).thenReturn(true) } fixture.scope.transaction = finished - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } + navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(navTransaction).isNotNull() assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isSameInstanceAs(finished) } @@ -257,9 +227,8 @@ class NavScopeTest { navScopes.withNavScope { it.startNavTransaction("/Profile") } val previous = fixture.startedTransactions.single() - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } + navScopes.withNavScope { it.startNavTransaction("/Settings") } - assertThat(navTransaction).isNull() verify(previous, never()).finish(any()) assertThat(fixture.startedTransactions).containsExactly(previous) assertThat(fixture.scope.transaction).isSameInstanceAs(previous) @@ -272,9 +241,8 @@ class NavScopeTest { navScopes.withNavScope { it.startNavTransaction("/Profile") } navScopes.withNavScope { it.finishNavTransaction() } - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } + navScopes.withNavScope { it.startNavTransaction("/Settings") } - assertThat(navTransaction).isNotNull() assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.last()) } @@ -282,13 +250,11 @@ class NavScopeTest { fun `startNavTransaction() - replaces the previous deferred transaction without the nav lease`() { val fixture = Fixture.create() val navScopes = fixture.navScopes - val previous = navScopes.withNavScope { it.startNavTransaction("/Profile") } as NavTransaction + navScopes.withNavScope { it.startNavTransaction("/Profile") } - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } + navScopes.withNavScope { it.startNavTransaction("/Settings") } fixture.acquireLease() - assertThat(navTransaction).isNotNull() - assertThat(previous.transaction).isNull() assertThat(fixture.startedTransactions.map { it.name }).containsExactly("/Settings") assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.single()) } @@ -303,9 +269,8 @@ class NavScopeTest { fixture.scope.transaction = hostTransaction navScopes.withNavScope { it.finishNavTransaction() } - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } + navScopes.withNavScope { it.startNavTransaction("/Settings") } - assertThat(navTransaction).isNull() verify(previous).finish(SpanStatus.OK) assertThat(fixture.startedTransactions).containsExactly(previous) assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) @@ -315,15 +280,13 @@ class NavScopeTest { fun `startNavTransaction() - replaces the previous deferred transaction while a host transaction is bound`() { val fixture = Fixture.create() val navScopes = fixture.navScopes - val previous = navScopes.withNavScope { it.startNavTransaction("/Profile") } as NavTransaction + navScopes.withNavScope { it.startNavTransaction("/Profile") } fixture.scope.transaction = mock() - val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } + navScopes.withNavScope { it.startNavTransaction("/Settings") } fixture.scope.clearTransaction() fixture.acquireLease() - assertThat(navTransaction).isNotNull() - assertThat(previous.transaction).isNull() assertThat(fixture.startedTransactions.map { it.name }).containsExactly("/Settings") assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.single()) } @@ -414,8 +377,9 @@ class NavScopeTest { return results.single() } - private fun INavScope.startNavTransaction(name: String): INavTransaction? = - startNavTransaction(TransactionContext(name, "navigation"), TransactionOptions()) + private fun INavScope.startNavTransaction(name: String) { + startNavTransaction(navTransactionRequest(name)) + } /** Thrown by a scope observer, to stand in for one the host app registered. */ private class ThrowingScopeObserverException : RuntimeException() @@ -470,7 +434,7 @@ class NavScopeTest { val other = resumedNavScopes() other.onBackStackUpdate() other.configureScope { - it.startNavTransaction(TransactionContext("/Other", "navigation"), TransactionOptions()) + it.startNavTransaction(navTransactionRequest("/Other")) } return startedTransactions.single { it.name == "/Other" } } @@ -492,3 +456,9 @@ class NavScopeTest { } } } + +private fun navTransactionRequest(name: String): NavTransactionRequest = + NavTransactionRequest( + name = name, + newOptions = ::TransactionOptions, + ) diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt index fe20e834ae8..fb9782877e2 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt @@ -10,7 +10,6 @@ import io.sentry.Scope import io.sentry.ScopeType import io.sentry.Scopes import io.sentry.SentryOptions -import io.sentry.TransactionContext import io.sentry.TransactionOptions import org.junit.Test import org.mockito.kotlin.mock @@ -37,7 +36,12 @@ class NavScopesIntegrationTest { 1.0 } incoming.configureScope { - it.startNavTransaction(TransactionContext("/Checkout", "navigation"), TransactionOptions()) + it.startNavTransaction( + NavTransactionRequest( + name = "/Checkout", + newOptions = ::TransactionOptions, + ) + ) } assertThat(contextsAtSampling).isEmpty() assertThat(fixture.scope.propagationContext).isSameInstanceAs(contextBeforeResume) diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt index a50ec5445c0..84e1d1822c9 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt @@ -15,13 +15,16 @@ import io.sentry.ScopeCallback import io.sentry.ScopeObserverAdapter import io.sentry.SentryLevel import io.sentry.SentryOptions +import io.sentry.SpanContext import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.TransactionOptions +import io.sentry.protocol.App import io.sentry.protocol.Contexts import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any +import org.mockito.kotlin.atLeastOnce import org.mockito.kotlin.doAnswer import org.mockito.kotlin.eq import org.mockito.kotlin.mock @@ -229,23 +232,22 @@ class NavScopesTest { navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() - val transaction = navScopes.startNavTransaction("/Home") + navScopes.startNavTransaction("/Home") val started = fixture.startedTransaction("/Home") - assertThat(transaction).isNotNull() assertThat(fixture.scope.transaction).isSameInstanceAs(started) } @Test - fun `startNavTransaction() - returns null when the delegate declines with the nav lease`() { + fun `startNavTransaction() - binds no transaction when the delegate declines with the nav lease`() { val fixture = Fixture.create(declinesTransactions = true) val navScopes = fixture.navScopes() navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() - val transaction = navScopes.startNavTransaction("/Home") + navScopes.startNavTransaction("/Home") - assertThat(transaction).isNull() + assertThat(fixture.scope.transaction).isNull() } @Test @@ -254,7 +256,7 @@ class NavScopesTest { val navScopes = fixture.navScopes() navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() - fixture.scope.transaction = mock() + fixture.scope.transaction = hostTransaction() navScopes.startNavTransaction("/Home") @@ -266,10 +268,9 @@ class NavScopesTest { val fixture = Fixture.create() val covered = fixture.coveredNavScopes() - val transaction = covered.startNavTransaction("/Result") + covered.startNavTransaction("/Result") assertThat(fixture.hasLease(covered)).isFalse() - assertThat(transaction).isNotNull() assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isNull() } @@ -337,7 +338,7 @@ class NavScopesTest { val navScopes = fixture.navScopes() navScopes.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) navScopes.applyFirstBackStackUpdate() - val hostTransaction = mock() + val hostTransaction = hostTransaction() fixture.scope.transaction = hostTransaction navScopes.finishNavTransaction() @@ -355,9 +356,8 @@ class NavScopesTest { navScopes.startNavTransaction("/Home") navScopes.finishNavTransaction() - val transaction = navScopes.startNavTransaction("/Profile") + navScopes.startNavTransaction("/Profile") - assertThat(transaction).isNotNull() assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) } @@ -402,7 +402,7 @@ class NavScopesTest { } @Test - fun `a finished nav transaction is discarded when the nav lease is regained`() { + fun `a finished nav transaction is not bound again when the nav lease is regained`() { val fixture = Fixture.create() val covered = fixture.coveredNavScopes(startedRoute = "/Home") val started = fixture.startedTransaction("/Home") @@ -411,7 +411,7 @@ class NavScopesTest { fixture.incoming.dispose() assertThat(fixture.hasLease(covered)).isTrue() - assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.transaction).isNotSameInstanceAs(started) } @Test @@ -424,6 +424,103 @@ class NavScopesTest { fixture.incoming.dispose() assertThat(fixture.hasLease(covered)).isTrue() + assertThat(fixture.startedTransactions).hasSize(1) + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `regaining the nav lease after the nav transaction finished starts a new one for its request`() { + val fixture = Fixture.create() + fixture.coveredNavScopes(startedRoute = "/Home") + val finished = fixture.startedTransaction("/Home") + whenever(finished.isFinished).thenReturn(true) + + fixture.incoming.dispose() + + val restarted = fixture.startedTransactions.last() + assertThat(fixture.startedTransactions).hasSize(2) + assertThat(restarted).isNotSameInstanceAs(finished) + assertThat(restarted.name).isEqualTo("/Home") + assertThat(fixture.scope.transaction).isSameInstanceAs(restarted) + } + + @Test + fun `regaining the nav lease after another transaction blocked the nav transaction starts one for its request`() { + val fixture = Fixture.create() + fixture.scope.transaction = hostTransaction() + fixture.coveredNavScopes(startedRoute = "/Home") + check(fixture.startedTransactions.isEmpty()) + fixture.scope.clearTransaction() + + fixture.incoming.dispose() + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Home")) + } + + @Test + fun `regaining the nav lease again after a host transaction blocked the deferred transaction starts one`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + val hostTransaction = hostTransaction() + fixture.scope.transaction = hostTransaction + fixture.incoming.dispose() + check(fixture.startedTransactions.isEmpty()) + fixture.scope.clearTransaction() + val nextIncoming = fixture.navScopes() + nextIncoming.attach(NavRootInfo(fixture.lifecycle(Lifecycle.State.RESUMED), activity = null)) + nextIncoming.applyFirstBackStackUpdate() + + nextIncoming.dispose() + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Result")) + } + + @Test + fun `regaining the nav lease with an unfinished nav transaction starts no new one`() { + val fixture = Fixture.create() + fixture.coveredNavScopes(startedRoute = "/Home") + val started = fixture.startedTransaction("/Home") + + fixture.incoming.dispose() + + assertThat(fixture.startedTransactions).containsExactly(started) + } + + @Test + fun `regaining the nav lease with a deferred transaction starts only the deferred one`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + + fixture.incoming.dispose() + + assertThat(fixture.startedTransactions.map { it.name }).containsExactly("/Result") + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Result")) + } + + @Test + fun `regaining the nav lease with a deferred transaction blocked by a host transaction starts none`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + val hostTransaction = hostTransaction() + fixture.scope.transaction = hostTransaction + + fixture.incoming.dispose() + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `regaining the nav lease without ever starting a nav transaction starts none`() { + val fixture = Fixture.create() + fixture.coveredNavScopes() + + fixture.incoming.dispose() + + assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isNull() } @@ -439,17 +536,33 @@ class NavScopesTest { } @Test - fun `a deferred transaction receives the data recorded on its handle once it starts`() { + fun `a deferred transaction is started from its request when the nav lease is acquired`() { val fixture = Fixture.create() val covered = fixture.coveredNavScopes() - val transaction = covered.startNavTransaction("/Result") - transaction?.setData("arguments", "id=1") + covered.startNavTransaction( + navTransactionRequest("/Result", data = mapOf("arguments" to "id=1")) + ) fixture.incoming.dispose() verify(fixture.startedTransaction("/Result")).setData("arguments", "id=1") } + @Test + fun `a deferred transaction copies the scope's app context as it is when the nav lease is acquired`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + fixture.scope.contexts.setApp(App().apply { appVersion = "1.0.0" }) + covered.startNavTransaction(navTransactionRequest("/Result", viewNames = listOf("/Result"))) + fixture.scope.contexts.setApp(App().apply { appVersion = "2.0.0" }) + + fixture.incoming.dispose() + + val app = fixture.startedTransaction("/Result").contexts.app + assertThat(app?.appVersion).isEqualTo("2.0.0") + assertThat(app?.viewNames).containsExactly("/Result") + } + @Test fun `a preloaded owner's deferred transaction starts when the owner resumes`() { val fixture = Fixture.create() @@ -520,7 +633,7 @@ class NavScopesTest { val fixture = Fixture.create() val covered = fixture.coveredNavScopes() covered.startNavTransaction("/Result") - val hostTransaction = mock() + val hostTransaction = hostTransaction() fixture.scope.transaction = hostTransaction fixture.incoming.dispose() @@ -534,11 +647,12 @@ class NavScopesTest { val fixture = Fixture.create() val covered = fixture.coveredNavScopes() covered.startNavTransaction("/Result") - fixture.scope.transaction = mock() + fixture.scope.transaction = hostTransaction() fixture.incoming.dispose() - verify(fixture.logger).log(eq(SentryLevel.DEBUG), any(), eq("/Result")) + // Logged again when the restart for the same request is blocked, too. + verify(fixture.logger, atLeastOnce()).log(eq(SentryLevel.DEBUG), any(), eq("/Result")) } @Test @@ -652,12 +766,13 @@ class NavScopesTest { } /** Starts a nav transaction for [name] through the [INavScope] handed out by [NavScopes]. */ - private fun NavScopes.startNavTransaction(name: String): INavTransaction? { - var transaction: INavTransaction? = null - configureScope { - transaction = it.startNavTransaction(TransactionContext(name, "op"), TransactionOptions()) - } - return transaction + private fun NavScopes.startNavTransaction(name: String) { + startNavTransaction(navTransactionRequest(name)) + } + + /** Starts a nav transaction for [request] through the [INavScope] handed out by [NavScopes]. */ + private fun NavScopes.startNavTransaction(request: NavTransactionRequest) { + configureScope { it.startNavTransaction(request) } } /** Finishes the nav transaction through the [INavScope] handed out by [NavScopes]. */ @@ -707,8 +822,12 @@ class NavScopesTest { NoOpTransaction.getInstance() } else { val name = (it.arguments[0] as TransactionContext).name + val contexts = Contexts() mock() - .also { transaction -> whenever(transaction.name).thenReturn(name) } + .also { transaction -> + whenever(transaction.name).thenReturn(name) + whenever(transaction.contexts).thenReturn(contexts) + } .also(startedTransactions::add) } } @@ -746,9 +865,7 @@ class NavScopesTest { covered.attach(NavRootInfo(lifecycle(Lifecycle.State.RESUMED), activity = null)) covered.onBackStackUpdate() if (startedRoute != null) { - covered.configureScope { - it.startNavTransaction(TransactionContext(startedRoute, "op"), TransactionOptions()) - } + covered.configureScope { it.startNavTransaction(navTransactionRequest(startedRoute)) } } incoming = navScopes() incoming.attach(NavRootInfo(lifecycle(Lifecycle.State.RESUMED), activity = null)) @@ -778,3 +895,19 @@ class NavScopesTest { } } } + +private fun navTransactionRequest( + name: String, + data: Map = emptyMap(), + viewNames: List? = null, +): NavTransactionRequest = + NavTransactionRequest( + name = name, + newOptions = ::TransactionOptions, + data = data, + viewNames = viewNames, + ) + +/** A transaction the host app bound to the scope, as identified by its manual origin. */ +private fun hostTransaction(): ITransaction = + mock().also { whenever(it.spanContext).thenReturn(SpanContext("host")) } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionRequestTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionRequestTest.kt new file mode 100644 index 00000000000..f7155b67878 --- /dev/null +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionRequestTest.kt @@ -0,0 +1,244 @@ +package io.sentry.compose.navigation3.lifecycle + +import com.google.common.truth.Truth.assertThat +import io.sentry.IScopes +import io.sentry.ITransaction +import io.sentry.NoOpTransaction +import io.sentry.Scope +import io.sentry.SentryOptions +import io.sentry.TransactionContext +import io.sentry.TransactionOptions +import io.sentry.protocol.App +import io.sentry.protocol.Contexts +import io.sentry.protocol.TransactionNameSource +import org.junit.Test +import org.mockito.kotlin.any +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever + +class NavTransactionRequestTest { + + private val navigationContext = mapOf("backstack" to listOf(mapOf("route" to "/Checkout"))) + + private val scope = Scope(SentryOptions()) + + @Test + fun `forDestination() - reads transaction options when each transaction starts`() { + val fixture = Fixture.create() + val options = SentryOptions().apply { idleTimeout = 100L } + val request = + NavTransactionRequest.forDestination( + name = "/Checkout", + arguments = mapOf("id" to "1"), + contexts = emptyMap(), + viewNames = null, + options = options, + ) + + options.idleTimeout = 200L + request.start(fixture.scopes, scope) + options.idleTimeout = 300L + request.start(fixture.scopes, scope) + + assertThat(fixture.startedOptions.map { it.idleTimeout }).containsExactly(200L, 300L).inOrder() + assertThat(fixture.startedOptions.map { it.origin }) + .containsExactly("auto.navigation.nav3", "auto.navigation.nav3") + verify(fixture.startedTransactions.first()).setData("arguments", mapOf("id" to "1")) + } + + @Test + fun `start() - starts a navigation transaction named by route with the request's name`() { + val fixture = Fixture.create() + val request = fixture.request() + + val transaction = request.start(fixture.scopes, scope) + + val context = fixture.startedContexts.single() + assertThat(transaction).isSameInstanceAs(fixture.startedTransactions.single()) + assertThat(context.name).isEqualTo("/Checkout") + assertThat(context.transactionNameSource).isEqualTo(TransactionNameSource.ROUTE) + assertThat(context.operation).isEqualTo("navigation") + } + + @Test + fun `start() - starts each transaction with a new transaction context`() { + val fixture = Fixture.create() + val request = fixture.request() + + request.start(fixture.scopes, scope) + request.start(fixture.scopes, scope) + + val (first, second) = fixture.startedContexts + assertThat(second).isNotSameInstanceAs(first) + assertThat(second.spanId).isNotEqualTo(first.spanId) + assertThat(second.traceId).isNotEqualTo(first.traceId) + } + + @Test + fun `start() - starts each transaction with new options from the options factory`() { + val fixture = Fixture.create() + val createdOptions = mutableListOf() + val request = fixture.request(newOptions = { TransactionOptions().also(createdOptions::add) }) + + request.start(fixture.scopes, scope) + request.start(fixture.scopes, scope) + + assertThat(createdOptions).hasSize(2) + assertThat(fixture.startedOptions).containsExactlyElementsIn(createdOptions).inOrder() + assertThat(createdOptions[1]).isNotSameInstanceAs(createdOptions[0]) + } + + @Test + fun `start() - sets the request's data on the transaction`() { + val fixture = Fixture.create() + val request = fixture.request(data = mapOf("arguments" to mapOf("id" to "1"))) + + val transaction = request.start(fixture.scopes, scope)!! + + verify(transaction).setData("arguments", mapOf("id" to "1")) + } + + @Test + fun `start() - sets the request's contexts on the transaction`() { + val fixture = Fixture.create() + val request = fixture.request(contexts = mapOf(NAVIGATION_CONTEXT_KEY to navigationContext)) + + val transaction = request.start(fixture.scopes, scope)!! + + verify(transaction).setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + } + + @Test + fun `start() - sets the view names on a copy of the scope's app context`() { + val fixture = Fixture.create() + val scopeApp = + App().apply { + appName = "Sample" + appVersion = "1.2.3" + viewNames = listOf("/Previous") + } + scope.contexts.setApp(scopeApp) + val request = fixture.request(viewNames = listOf("/Checkout")) + + val transaction = request.start(fixture.scopes, scope)!! + + val transactionApp = transaction.contexts.app + assertThat(transactionApp).isNotSameInstanceAs(scopeApp) + assertThat(transactionApp?.viewNames).containsExactly("/Checkout") + assertThat(transactionApp?.appName).isEqualTo("Sample") + assertThat(transactionApp?.appVersion).isEqualTo("1.2.3") + assertThat(scope.contexts.app?.viewNames).containsExactly("/Previous") + } + + @Test + fun `start() - copies the scope's app context anew for each transaction`() { + val fixture = Fixture.create() + val request = fixture.request(viewNames = listOf("/Checkout")) + scope.contexts.setApp(App().apply { appVersion = "1.0.0" }) + request.start(fixture.scopes, scope) + scope.contexts.setApp(App().apply { appVersion = "2.0.0" }) + + val transaction = request.start(fixture.scopes, scope)!! + + assertThat(transaction.contexts.app?.appVersion).isEqualTo("2.0.0") + } + + @Test + fun `start() - sets the view names on a new app context when the scope has none`() { + val fixture = Fixture.create() + val request = fixture.request(viewNames = listOf("/Checkout")) + + val transaction = request.start(fixture.scopes, scope)!! + + assertThat(transaction.contexts.app?.viewNames).containsExactly("/Checkout") + assertThat(scope.contexts.app).isNull() + } + + @Test + fun `start() - leaves the transaction's app context alone without view names`() { + val fixture = Fixture.create() + scope.contexts.setApp(App().apply { appName = "Sample" }) + val request = fixture.request(viewNames = null) + + val transaction = request.start(fixture.scopes, scope)!! + + assertThat(transaction.contexts.app).isNull() + } + + @Test + fun `start() - returns null when the scopes start a no-op transaction`() { + val fixture = Fixture.create(declinesTransactions = true) + val request = + fixture.request( + data = mapOf("arguments" to "id=1"), + contexts = mapOf(NAVIGATION_CONTEXT_KEY to navigationContext), + viewNames = listOf("/Checkout"), + ) + + val transaction = request.start(fixture.scopes, scope) + + assertThat(transaction).isNull() + } + + @Test + fun `start() - sets nothing on a no-op transaction`() { + val fixture = Fixture.create() + val noOpTransaction = + mock().also { + whenever(it.isNoOp).thenReturn(true) + whenever(fixture.scopes.startTransaction(any(), any())).thenReturn(it) + } + val request = fixture.request(data = mapOf("arguments" to "id=1")) + + request.start(fixture.scopes, scope) + + verify(noOpTransaction, never()).setData(any(), any()) + } + + private class Fixture private constructor() { + /** Whether [scopes] declines to start transactions, returning a no-op transaction instead. */ + var declinesTransactions = false + + val startedContexts = mutableListOf() + val startedOptions = mutableListOf() + val startedTransactions = mutableListOf() + + val scopes = + mock().also { scopes -> + whenever(scopes.startTransaction(any(), any())) + .thenAnswer { + if (declinesTransactions) { + NoOpTransaction.getInstance() + } else { + startedContexts += it.arguments[0] as TransactionContext + startedOptions += it.arguments[1] as TransactionOptions + val contexts = Contexts() + mock() + .also { transaction -> whenever(transaction.contexts).thenReturn(contexts) } + .also(startedTransactions::add) + } + } + } + + fun request( + newOptions: () -> TransactionOptions = ::TransactionOptions, + data: Map = emptyMap(), + contexts: Map = emptyMap(), + viewNames: List? = null, + ): NavTransactionRequest = + NavTransactionRequest( + name = "/Checkout", + newOptions = newOptions, + data = data, + contexts = contexts, + viewNames = viewNames, + ) + + companion object { + fun create(declinesTransactions: Boolean = false): Fixture = + Fixture().also { it.declinesTransactions = declinesTransactions } + } + } +} diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt deleted file mode 100644 index 1eca057f395..00000000000 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt +++ /dev/null @@ -1,348 +0,0 @@ -package io.sentry.compose.navigation3.lifecycle - -import com.google.common.truth.Truth.assertThat -import io.sentry.IScopes -import io.sentry.ITransaction -import io.sentry.NoOpTransaction -import io.sentry.SpanStatus -import io.sentry.TransactionContext -import io.sentry.TransactionOptions -import io.sentry.protocol.App -import io.sentry.protocol.Contexts -import org.junit.Test -import org.mockito.kotlin.any -import org.mockito.kotlin.inOrder -import org.mockito.kotlin.mock -import org.mockito.kotlin.never -import org.mockito.kotlin.verify -import org.mockito.kotlin.verifyNoInteractions -import org.mockito.kotlin.whenever - -class NavTransactionTest { - - private val transactionContext = TransactionContext("/Checkout", "navigation") - private val transactionOptions = TransactionOptions() - - private val app = App().apply { viewNames = listOf("/Checkout") } - private val navigationContext = mapOf("Back Stack" to listOf(mapOf("entry" to "/Checkout"))) - - @Test - fun `started() - is named after the transaction`() { - val fixture = Fixture.create() - - val handle = NavTransaction.started(fixture.transaction) - - assertThat(handle.name).isEqualTo("/Checkout") - } - - @Test - fun `deferred() - is named after the transaction context`() { - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - - assertThat(handle.name).isEqualTo("/Checkout") - } - - @Test - fun `transaction - is null for a deferred handle`() { - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - - assertThat(handle.transaction).isNull() - } - - @Test - fun `transaction - is the backing transaction for a started handle`() { - val fixture = Fixture.create() - - val handle = NavTransaction.started(fixture.transaction) - - assertThat(handle.transaction).isSameInstanceAs(fixture.transaction) - } - - @Test - fun `transaction - is the started transaction once a deferred handle has started`() { - val fixture = Fixture.create() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - - handle.start(fixture.scopes) - - assertThat(handle.transaction).isSameInstanceAs(fixture.transaction) - } - - @Test - fun `transaction - stays null when the scopes decline to start a deferred handle's transaction`() { - val fixture = Fixture.create(declinesTransactions = true) - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - - handle.start(fixture.scopes) - - assertThat(handle.transaction).isNull() - } - - @Test - fun `transaction - is null once a deferred handle is finished before it starts`() { - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - - handle.finish(onFinished = {}) - - assertThat(handle.transaction).isNull() - } - - @Test - fun `a started handle sets data, contexts, and the app context on the transaction`() { - val fixture = Fixture.create() - val handle = NavTransaction.started(fixture.transaction) - - handle.setData("arguments", "id=1") - handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) - handle.setApp(app) - - verify(fixture.transaction).setData("arguments", "id=1") - verify(fixture.transaction).setContext(NAVIGATION_CONTEXT_KEY, navigationContext) - assertThat(fixture.transaction.contexts.app).isSameInstanceAs(app) - } - - @Test - fun `a deferred handle sets data, contexts, and the app context on the transaction once it has started`() { - val fixture = Fixture.create() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - handle.start(fixture.scopes) - - handle.setData("arguments", "id=1") - handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) - handle.setApp(app) - - verify(fixture.transaction).setData("arguments", "id=1") - verify(fixture.transaction).setContext(NAVIGATION_CONTEXT_KEY, navigationContext) - assertThat(fixture.transaction.contexts.app).isSameInstanceAs(app) - } - - @Test - fun `start() - starts and returns the transaction with the deferred context and options`() { - val fixture = Fixture.create() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - - val started = handle.start(fixture.scopes) - - verify(fixture.scopes).startTransaction(transactionContext, transactionOptions) - assertThat(started).isSameInstanceAs(fixture.transaction) - } - - @Test - fun `start() - replays data, contexts, and the app context set before the start`() { - val fixture = Fixture.create() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - handle.setData("arguments", "id=1") - handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) - handle.setApp(app) - - handle.start(fixture.scopes) - - verify(fixture.transaction).setData("arguments", "id=1") - verify(fixture.transaction).setContext(NAVIGATION_CONTEXT_KEY, navigationContext) - assertThat(fixture.transaction.contexts.app).isSameInstanceAs(app) - } - - @Test - fun `start() - replays only the latest value set for each key`() { - val fixture = Fixture.create() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - val latestNavigationContext = mapOf("Back Stack" to listOf(mapOf("entry" to "/Receipt"))) - val latestApp = App().apply { viewNames = listOf("/Receipt") } - handle.setData("arguments", "id=1") - handle.setData("arguments", "id=2") - handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) - handle.setContext(NAVIGATION_CONTEXT_KEY, latestNavigationContext) - handle.setApp(app) - handle.setApp(latestApp) - - handle.start(fixture.scopes) - - verify(fixture.transaction, never()).setData("arguments", "id=1") - verify(fixture.transaction).setData("arguments", "id=2") - verify(fixture.transaction, never()).setContext(NAVIGATION_CONTEXT_KEY, navigationContext) - verify(fixture.transaction).setContext(NAVIGATION_CONTEXT_KEY, latestNavigationContext) - assertThat(fixture.transaction.contexts.app).isSameInstanceAs(latestApp) - } - - @Test - fun `start() - returns null when the scopes decline to start a transaction`() { - val fixture = Fixture.create(declinesTransactions = true) - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - - val started = handle.start(fixture.scopes) - - assertThat(started).isNull() - } - - @Test - fun `start() - starts the transaction on a retry after the scopes declined to start one`() { - val fixture = Fixture.create(declinesTransactions = true) - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - handle.setData("arguments", "id=1") - handle.start(fixture.scopes) - fixture.declinesTransactions = false - - val started = handle.start(fixture.scopes) - - assertThat(started).isSameInstanceAs(fixture.transaction) - verify(fixture.transaction).setData("arguments", "id=1") - } - - @Test - fun `start() - starts no transaction a second time`() { - val fixture = Fixture.create() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - handle.start(fixture.scopes) - - val started = handle.start(fixture.scopes) - - assertThat(started).isNull() - verify(fixture.scopes).startTransaction(any(), any()) - } - - @Test - fun `start() - starts no transaction for a started handle`() { - val fixture = Fixture.create() - val handle = NavTransaction.started(fixture.transaction) - - val started = handle.start(fixture.scopes) - - assertThat(started).isNull() - verify(fixture.scopes, never()).startTransaction(any(), any()) - } - - @Test - fun `finish() - finishes a started handle's transaction with the transaction's status`() { - val fixture = Fixture.create() - whenever(fixture.transaction.status).thenReturn(SpanStatus.CANCELLED) - val handle = NavTransaction.started(fixture.transaction) - - handle.finish(onFinished = {}) - - verify(fixture.transaction).finish(SpanStatus.CANCELLED) - } - - @Test - fun `finish() - finishes a started handle's transaction with OK when the transaction has no status`() { - val fixture = Fixture.create() - val handle = NavTransaction.started(fixture.transaction) - - handle.finish(onFinished = {}) - - verify(fixture.transaction).finish(SpanStatus.OK) - } - - @Test - fun `finish() - passes a started handle's transaction to the callback after finishing it`() { - val fixture = Fixture.create() - val onFinished = mock<(ITransaction) -> Unit>() - val handle = NavTransaction.started(fixture.transaction) - - handle.finish(onFinished) - - inOrder(fixture.transaction, onFinished) { - verify(fixture.transaction).finish(SpanStatus.OK) - verify(onFinished).invoke(fixture.transaction) - } - } - - @Test - fun `finish() - doesn't finish a started handle's transaction that has already finished`() { - val fixture = Fixture.create() - whenever(fixture.transaction.isFinished).thenReturn(true) - val handle = NavTransaction.started(fixture.transaction) - - handle.finish(onFinished = {}) - - verify(fixture.transaction, never()).finish(any()) - } - - @Test - fun `finish() - passes a started handle's already finished transaction to the callback`() { - val fixture = Fixture.create() - val onFinished = mock<(ITransaction) -> Unit>() - whenever(fixture.transaction.isFinished).thenReturn(true) - val handle = NavTransaction.started(fixture.transaction) - - handle.finish(onFinished) - - verify(onFinished).invoke(fixture.transaction) - } - - @Test - fun `finish() - finishes a deferred handle's transaction once it has started`() { - val fixture = Fixture.create() - val onFinished = mock<(ITransaction) -> Unit>() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - handle.start(fixture.scopes) - - handle.finish(onFinished) - - verify(fixture.transaction).finish(SpanStatus.OK) - verify(onFinished).invoke(fixture.transaction) - } - - @Test - fun `finish() - discards a pending handle so that it never starts a transaction`() { - val fixture = Fixture.create() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - - handle.finish(onFinished = {}) - val started = handle.start(fixture.scopes) - - assertThat(started).isNull() - verify(fixture.scopes, never()).startTransaction(any(), any()) - } - - @Test - fun `finish() - does not call back for a pending handle`() { - val onFinished = mock<(ITransaction) -> Unit>() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - - handle.finish(onFinished) - - verifyNoInteractions(onFinished) - } - - @Test - fun `a discarded handle ignores later writes and finishes`() { - val fixture = Fixture.create() - val onFinished = mock<(ITransaction) -> Unit>() - val handle = NavTransaction.deferred(transactionContext, transactionOptions) - handle.finish(onFinished = {}) - - handle.setData("arguments", "id=1") - handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) - handle.setApp(app) - handle.finish(onFinished) - val started = handle.start(fixture.scopes) - - assertThat(started).isNull() - verifyNoInteractions(fixture.transaction, onFinished) - } - - private class Fixture private constructor(val scopes: IScopes, val transaction: ITransaction) { - - /** Whether [scopes] declines to start transactions, returning a no-op transaction instead. */ - var declinesTransactions = false - - companion object { - fun create(declinesTransactions: Boolean = false): Fixture { - val contexts = Contexts() - val transaction = - mock().also { - whenever(it.name).thenReturn("/Checkout") - whenever(it.contexts).thenReturn(contexts) - } - val scopes = mock() - val fixture = Fixture(scopes, transaction) - fixture.declinesTransactions = declinesTransactions - whenever(scopes.startTransaction(any(), any())) - .thenAnswer { - if (fixture.declinesTransactions) NoOpTransaction.getInstance() else transaction - } - return fixture - } - } - } -} diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt index e732db751e9..0488c7d1c17 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt @@ -9,6 +9,7 @@ import io.sentry.Scope import io.sentry.ScopeObserverAdapter import io.sentry.SentryLevel import io.sentry.SentryOptions +import io.sentry.SpanContext import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.TransactionOptions @@ -24,232 +25,267 @@ import org.mockito.kotlin.whenever @Suppress("LargeClass") class StagedNavTransactionTest { - private val transactionContext = TransactionContext("/Profile", "navigation") - private val transactionOptions = TransactionOptions() + private val request = navTransactionRequest("/Profile") + private val updatedContexts = mapOf("Navigation" to "updated") private val finishedTransaction = mock().also { whenever(it.isFinished).thenReturn(true) } - private val hostTransaction = mock() + private val hostTransaction = transactionWithOrigin("manual") @Test - fun `start() - starts a nav transaction and binds it to a free scope`() { + fun `publishTo() - keeps the started nav transaction when binding it throws`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.transactionObserverThrows = true - val navTransaction = - stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) - + assertThrows(ThrowingScopeObserverException::class.java) { + stagedNavTransaction.publishTo(fixture.scope) + } + fixture.transactionObserverThrows = false val started = fixture.startedTransaction("/Profile") - verify(fixture.scopes).startTransaction(transactionContext, transactionOptions) - assertThat(navTransaction?.transaction).isSameInstanceAs(started) - assertThat(fixture.scope.transaction).isSameInstanceAs(started) + stagedNavTransaction.finish(fixture.scope) + + verify(started).finish(SpanStatus.OK) } @Test - fun `start() - starts no nav transaction while another transaction is bound`() { + fun `stage() - starts no transaction`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - fixture.scope.transaction = hostTransaction - val navTransaction = - stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + stagedNavTransaction.stage(request) - assertThat(navTransaction).isNull() assertThat(fixture.startedTransactions).isEmpty() - assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + assertThat(fixture.scope.transaction).isNull() } @Test - fun `start() - logs the route when another transaction blocks the start`() { + fun `stage() - replaces the current nav transaction without finishing it`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - fixture.scope.transaction = hostTransaction + val current = fixture.start("/Home") - stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + stagedNavTransaction.stage(request) + stagedNavTransaction.finish(fixture.scope) - verify(fixture.logger).log(eq(SentryLevel.DEBUG), any(), eq("/Profile")) + verify(current, never()).finish(any()) } @Test - fun `start() - binds the nav transaction in place of a finished bound transaction`() { + fun `publishTo() - starts a staged nav transaction and binds it to a free scope`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - fixture.scope.transaction = finishedTransaction + stagedNavTransaction.stage(request) - val navTransaction = - stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + stagedNavTransaction.publishTo(fixture.scope) - val started = fixture.startedTransaction("/Profile") - assertThat(navTransaction?.transaction).isSameInstanceAs(started) - assertThat(fixture.scope.transaction).isSameInstanceAs(started) + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) } @Test - fun `start() - returns null and keeps the current nav transaction when the start is declined`() { + fun `publishTo() - starts a staged nav transaction in place of a finished bound transaction`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - val current = fixture.start("/Home") - fixture.declinesTransactions = true + stagedNavTransaction.stage(request) + fixture.scope.transaction = finishedTransaction - val navTransaction = - stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + stagedNavTransaction.publishTo(fixture.scope) - assertThat(navTransaction).isNull() - assertThat(fixture.scope.transaction).isSameInstanceAs(current) - stagedNavTransaction.finish(fixture.scope) - verify(current).finish(SpanStatus.OK) + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) } @Test - fun `start() - replaces the current nav transaction without finishing it`() { + fun `publishTo() - doesn't start a staged nav transaction while another transaction is bound`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - val current = fixture.start("/Home") - // Unbound, so that it doesn't block the start. - stagedNavTransaction.unpublishFrom(fixture.scope) + stagedNavTransaction.stage(request) + fixture.scope.transaction = hostTransaction - stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) - verify(current, never()).finish(any()) + stagedNavTransaction.publishTo(fixture.scope) - stagedNavTransaction.finish(fixture.scope) - verify(fixture.startedTransaction("/Profile")).finish(SpanStatus.OK) - verify(current, never()).finish(any()) + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } @Test - fun `start() - keeps the started nav transaction when binding it throws`() { + fun `publishTo() - logs the route when another transaction blocks the nav transaction`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - fixture.transactionObserverThrows = true + stagedNavTransaction.stage(request) + fixture.scope.transaction = hostTransaction - assertThrows(ThrowingScopeObserverException::class.java) { - stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) - } - fixture.transactionObserverThrows = false - val started = fixture.startedTransaction("/Profile") - stagedNavTransaction.finish(fixture.scope) + stagedNavTransaction.publishTo(fixture.scope) - verify(started).finish(SpanStatus.OK) + verify(fixture.logger).log(eq(SentryLevel.DEBUG), any(), eq("/Profile")) } @Test - fun `stage() - returns a pending nav transaction without starting a transaction`() { - val fixture = Fixture.create() + fun `publishTo() - binds nothing when a staged nav transaction's start is declined`() { + val fixture = Fixture.create(declinesTransactions = true) val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) - val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + stagedNavTransaction.publishTo(fixture.scope) - assertThat(navTransaction.transaction).isNull() - assertThat(navTransaction.name).isEqualTo("/Profile") - assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isNull() } @Test - fun `stage() - replaces the current nav transaction without finishing it`() { + fun `publishTo() - starts a nav transaction for a request whose staged one was declined`() { + val fixture = Fixture.create(declinesTransactions = true) + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + stagedNavTransaction.publishTo(fixture.scope) + fixture.declinesTransactions = false + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `publishTo() - binds an unpublished unfinished nav transaction again`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - val current = fixture.start("/Home") + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) - stagedNavTransaction.stage(transactionContext, transactionOptions) - stagedNavTransaction.finish(fixture.scope) + stagedNavTransaction.publishTo(fixture.scope) - verify(current, never()).finish(any()) + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + assertThat(fixture.startedTransactions).containsExactly(started) } @Test - fun `publishTo() - starts a staged nav transaction and binds it to a free scope`() { + fun `publishTo() - keeps the bound nav transaction bound`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + val started = fixture.start() stagedNavTransaction.publishTo(fixture.scope) - val started = fixture.startedTransaction("/Profile") - assertThat(navTransaction.transaction).isSameInstanceAs(started) assertThat(fixture.scope.transaction).isSameInstanceAs(started) + verify(fixture.logger, never()).log(eq(SentryLevel.DEBUG), any(), any()) } @Test - fun `publishTo() - starts a staged nav transaction in place of a finished bound transaction`() { + fun `publishTo() - binds an unpublished nav transaction in place of a finished bound transaction`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - stagedNavTransaction.stage(transactionContext, transactionOptions) + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) fixture.scope.transaction = finishedTransaction stagedNavTransaction.publishTo(fixture.scope) - assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + assertThat(fixture.scope.transaction).isSameInstanceAs(started) } @Test - fun `publishTo() - doesn't start a staged nav transaction while another transaction is bound`() { + fun `publishTo() - doesn't bind an unpublished nav transaction while another transaction is bound`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - stagedNavTransaction.stage(transactionContext, transactionOptions) + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) fixture.scope.transaction = hostTransaction stagedNavTransaction.publishTo(fixture.scope) - assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + stagedNavTransaction.finish(fixture.scope) + verify(started).finish(SpanStatus.OK) } @Test - fun `publishTo() - logs the route when another transaction blocks the nav transaction`() { + fun `publishTo() - forgets an unpublished nav transaction that has finished and keeps another transaction`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - stagedNavTransaction.stage(transactionContext, transactionOptions) + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + whenever(started.isFinished).thenReturn(true) fixture.scope.transaction = hostTransaction stagedNavTransaction.publishTo(fixture.scope) - verify(fixture.logger).log(eq(SentryLevel.DEBUG), any(), eq("/Profile")) + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `publishTo() - leaves the scope untouched without a nav transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.scope.transaction = finishedTransaction + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(finishedTransaction) + } + + @Test + fun `publishTo() - starts a new nav transaction for the last request once the last one finished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val finished = fixture.start() + whenever(finished.isFinished).thenReturn(true) + + stagedNavTransaction.publishTo(fixture.scope) + + val restarted = fixture.startedTransactions.last() + assertThat(fixture.startedTransactions).hasSize(2) + assertThat(restarted).isNotSameInstanceAs(finished) + assertThat(restarted.name).isEqualTo("/Profile") + assertThat(fixture.scope.transaction).isSameInstanceAs(restarted) } @Test - fun `publishTo() - discards a staged nav transaction blocked by another transaction`() { + fun `publishTo() - starts a nav transaction for a request whose staged one was discarded`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + stagedNavTransaction.stage(request) fixture.scope.transaction = hostTransaction stagedNavTransaction.publishTo(fixture.scope) fixture.scope.clearTransaction() stagedNavTransaction.publishTo(fixture.scope) - assertThat(navTransaction.start(fixture.scopes)).isNull() - assertThat(fixture.startedTransactions).isEmpty() - assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) } @Test - fun `publishTo() - binds nothing when a staged nav transaction's start is declined`() { - val fixture = Fixture.create(declinesTransactions = true) + fun `publishTo() - starts a nav transaction for the most recent of several requests`() { + val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - stagedNavTransaction.stage(transactionContext, transactionOptions) + fixture.scope.transaction = hostTransaction + stagedNavTransaction.stage(navTransactionRequest("/Home")) + stagedNavTransaction.publishTo(fixture.scope) + stagedNavTransaction.stage(request) + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() stagedNavTransaction.publishTo(fixture.scope) - assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + assertThat(fixture.startedTransactions).hasSize(1) } @Test - fun `publishTo() - discards a staged nav transaction whose start was declined`() { - val fixture = Fixture.create(declinesTransactions = true) + fun `publishTo() - makes the restarted nav transaction the one that finish() finishes`() { + val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - stagedNavTransaction.stage(transactionContext, transactionOptions) + val finished = fixture.start() + whenever(finished.isFinished).thenReturn(true) stagedNavTransaction.publishTo(fixture.scope) - fixture.declinesTransactions = false + val restarted = fixture.startedTransactions.last() - stagedNavTransaction.publishTo(fixture.scope) + stagedNavTransaction.finish(fixture.scope) - assertThat(fixture.startedTransactions).isEmpty() + verify(restarted).finish(SpanStatus.OK) assertThat(fixture.scope.transaction).isNull() } @Test - fun `publishTo() - binds an unpublished unfinished nav transaction again`() { + fun `publishTo() - starts no nav transaction while the started one is unfinished`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction val started = fixture.start() @@ -257,85 +293,144 @@ class StagedNavTransactionTest { stagedNavTransaction.publishTo(fixture.scope) - assertThat(fixture.scope.transaction).isSameInstanceAs(started) assertThat(fixture.startedTransactions).containsExactly(started) } @Test - fun `publishTo() - keeps the bound nav transaction bound`() { + fun `publishTo() - starts no nav transaction without a request`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - val started = fixture.start() stagedNavTransaction.publishTo(fixture.scope) - assertThat(fixture.scope.transaction).isSameInstanceAs(started) - verify(fixture.logger, never()).log(eq(SentryLevel.DEBUG), any(), any()) + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() } @Test - fun `publishTo() - binds an unpublished nav transaction in place of a finished bound transaction`() { + fun `updateRequest() - restarts the nav transaction from the updated request`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - val started = fixture.start() - stagedNavTransaction.unpublishFrom(fixture.scope) - fixture.scope.transaction = finishedTransaction + val finished = fixture.start() + whenever(finished.isFinished).thenReturn(true) + stagedNavTransaction.updateRequest( + navTransactionRequest("/Profile", contexts = updatedContexts) + ) stagedNavTransaction.publishTo(fixture.scope) + verify(fixture.startedTransactions.last()).setContext("Navigation", "updated") + } + + @Test + fun `updateRequest() - starts a staged nav transaction from the updated request`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + + stagedNavTransaction.updateRequest( + navTransactionRequest("/Profile", contexts = updatedContexts) + ) + stagedNavTransaction.publishTo(fixture.scope) + + val started = fixture.startedTransaction("/Profile") + verify(started).setContext("Navigation", "updated") assertThat(fixture.scope.transaction).isSameInstanceAs(started) } @Test - fun `publishTo() - doesn't bind an unpublished nav transaction while another transaction is bound`() { + fun `updateRequest() - leaves a started nav transaction alone`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction val started = fixture.start() - stagedNavTransaction.unpublishFrom(fixture.scope) - fixture.scope.transaction = hostTransaction + stagedNavTransaction.updateRequest( + navTransactionRequest("/Profile", contexts = updatedContexts) + ) stagedNavTransaction.publishTo(fixture.scope) - assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + assertThat(fixture.startedTransactions).containsExactly(started) + verify(started, never()).setContext(any(), any()) + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + } + + @Test + fun `updateRequest() - keeps no request after the nav transaction is finished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.start() stagedNavTransaction.finish(fixture.scope) - verify(started).finish(SpanStatus.OK) + + stagedNavTransaction.updateRequest( + navTransactionRequest("/Profile", contexts = updatedContexts) + ) + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.startedTransactions).hasSize(1) + assertThat(fixture.scope.transaction).isNull() } @Test - fun `publishTo() - forgets and unbinds a bound nav transaction that has finished`() { + fun `updateRequest() - keeps no request without a request`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + + stagedNavTransaction.updateRequest(request) + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `publishTo() - starts no nav transaction after the nav transaction is finished`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction val started = fixture.start() - whenever(started.isFinished).thenReturn(true) + stagedNavTransaction.finish(fixture.scope) stagedNavTransaction.publishTo(fixture.scope) + assertThat(fixture.startedTransactions).containsExactly(started) assertThat(fixture.scope.transaction).isNull() } @Test - fun `publishTo() - forgets an unpublished nav transaction that has finished and keeps another transaction`() { + fun `publishTo() - starts no nav transaction after a staged nav transaction is finished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + stagedNavTransaction.finish(fixture.scope) + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `publishTo() - starts no nav transaction after the nav transaction is forgotten`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction val started = fixture.start() - stagedNavTransaction.unpublishFrom(fixture.scope) whenever(started.isFinished).thenReturn(true) - fixture.scope.transaction = hostTransaction + stagedNavTransaction.forget() stagedNavTransaction.publishTo(fixture.scope) - assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + assertThat(fixture.startedTransactions).containsExactly(started) } @Test - fun `publishTo() - leaves the scope untouched without a nav transaction`() { + fun `publishTo() - starts no nav transaction while another transaction is bound`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - fixture.scope.transaction = finishedTransaction + val finished = fixture.start() + whenever(finished.isFinished).thenReturn(true) + fixture.scope.transaction = hostTransaction stagedNavTransaction.publishTo(fixture.scope) - assertThat(fixture.scope.transaction).isSameInstanceAs(finishedTransaction) + assertThat(fixture.startedTransactions).containsExactly(finished) + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } @Test @@ -367,13 +462,13 @@ class StagedNavTransactionTest { fun `unpublishFrom() - keeps the bound transaction while the nav transaction is staged`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + stagedNavTransaction.stage(request) fixture.scope.transaction = hostTransaction stagedNavTransaction.unpublishFrom(fixture.scope) assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) - assertThat(navTransaction.transaction).isNull() + assertThat(fixture.startedTransactions).isEmpty() } @Test @@ -388,6 +483,31 @@ class StagedNavTransactionTest { assertThat(fixture.scope.transaction).isNull() } + @Test + fun `finish() - finishes the nav transaction with its own status`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + whenever(started.status).thenReturn(SpanStatus.CANCELLED) + + stagedNavTransaction.finish(fixture.scope) + + verify(started).finish(SpanStatus.CANCELLED) + } + + @Test + fun `finish() - unbinds a nav transaction that already finished without finishing it again`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + whenever(started.isFinished).thenReturn(true) + + stagedNavTransaction.finish(fixture.scope) + + verify(started, never()).finish(any()) + assertThat(fixture.scope.transaction).isNull() + } + @Test fun `finish() - finishes an unpublished nav transaction and keeps another bound transaction`() { val fixture = Fixture.create() @@ -418,12 +538,11 @@ class StagedNavTransactionTest { fun `finish() - discards a staged nav transaction so that publishing never starts it`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + stagedNavTransaction.stage(request) stagedNavTransaction.finish(fixture.scope) stagedNavTransaction.publishTo(fixture.scope) - assertThat(navTransaction.start(fixture.scopes)).isNull() assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isNull() } @@ -473,7 +592,7 @@ class StagedNavTransactionTest { fun `forget() - forgets a staged nav transaction so that publishing never starts it`() { val fixture = Fixture.create() val stagedNavTransaction = fixture.stagedNavTransaction - stagedNavTransaction.stage(transactionContext, transactionOptions) + stagedNavTransaction.stage(request) stagedNavTransaction.forget() stagedNavTransaction.publishTo(fixture.scope) @@ -485,6 +604,11 @@ class StagedNavTransactionTest { /** Thrown by a scope observer, to stand in for one the host app registered. */ private class ThrowingScopeObserverException : RuntimeException() + private fun transactionWithOrigin(origin: String): ITransaction = + mock().also { + whenever(it.spanContext).thenReturn(SpanContext("op").apply { this.origin = origin }) + } + private class Fixture private constructor() { val scope = Scope(SentryOptions()) val logger = mock() @@ -538,11 +662,8 @@ class StagedNavTransactionTest { /** Starts a nav transaction for [name], binding it if the scope is free, and returns it. */ fun start(name: String = "/Profile"): ITransaction { - stagedNavTransaction.start( - scope, - TransactionContext(name, "navigation"), - TransactionOptions(), - ) + stagedNavTransaction.stage(navTransactionRequest(name)) + stagedNavTransaction.publishTo(scope) return startedTransaction(name) } @@ -552,3 +673,13 @@ class StagedNavTransactionTest { } } } + +private fun navTransactionRequest( + name: String, + contexts: Map = emptyMap(), +): NavTransactionRequest = + NavTransactionRequest( + name = name, + newOptions = ::TransactionOptions, + contexts = contexts, + ) From 5ec0c141f70f0f07095eaf280f4de41b49199a47 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Fri, 13 Nov 2026 08:25:13 +0100 Subject: [PATCH 4/5] fix(android): [Part 3] Stop attributing Nav3 data to screens without a SentryNavEffect (#6483) When an Activity or ViewPager2 page without a SentryNavEffect replaced the Nav3 leaseholder, the leaseholder kept its nav context and bound nav transaction, so errors and spans from the new screen were attributed to it. The coordinator now withdraws the lease in that case: it unpublishes the leaseholder's nav state and unbinds its transaction without handing the lease to another nav root. The leaseholder republishes them when it resumes. Withdrawal is triggered when another Activity resumes while the leaseholder isn't resumed (unless that Activity is itself a nav root), or when the leaseholder pauses while its host Activity stays resumed (e.g., a ViewPager2 swipe). The host Activity comes from NavRootInfo, and a NavActivityWatcher reports Activity resumes. A nav root that isn't resumed also no longer takes the lease on its first update while its host Activity is resumed (e.g., a page preloaded next to one without an effect). --- CHANGELOG.md | 4 + sentry-android-navigation3/build.gradle.kts | 4 + .../lifecycle/NavActivityWatcher.kt | 52 ++ .../lifecycle/NavLeaseCoordinator.kt | 139 +++- .../compose/navigation3/lifecycle/README.md | 42 +- .../lifecycle/StagedNavTransaction.kt | 1 + ...SentryNavEffectLifecycleIntegrationTest.kt | 362 ++++++++++- .../navigation3/SentryNavEffectTest.kt | 33 +- .../lifecycle/NavLeaseCoordinatorTest.kt | 600 +++++++++++++++++- .../src/main/AndroidManifest.xml | 5 + .../navigation/nav3/Nav3MultiFragment.kt | 17 +- .../navigation/nav3/Nav3PlainScreens.kt | 206 ++++++ .../navigation/nav3/Nav3RouteScreens.kt | 1 + .../android/navigation/nav3/Nav3Routes.kt | 4 +- .../navigation/nav3/Nav3TransactionHistory.kt | 16 + 15 files changed, 1440 insertions(+), 46 deletions(-) create mode 100644 sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt create mode 100644 sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3PlainScreens.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index f46da095067..57a00e38a08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,10 @@ - Nav3: Returning to an Activity or Fragment with its own `SentryNavEffect` within the app (e.g., closing the Activity that covered it, swiping back to its ViewPager2 page, or dismissing a dialog over it) now starts a new navigation transaction for the current destination if the previous one finished, so spans produced upon return can now be consistently captured ([#6482](https://github.com/getsentry/sentry-java/pull/6482)) +### Fixes + +- Nav3: Stop attributing errors and spans from an Activity or ViewPager2 page without a `SentryNavEffect` to the most recently visited Nav3 root ([#6483](https://github.com/getsentry/sentry-java/pull/6483)) + ## 8.63.0 ### Features diff --git a/sentry-android-navigation3/build.gradle.kts b/sentry-android-navigation3/build.gradle.kts index 7a95528c32b..6e8814f8641 100644 --- a/sentry-android-navigation3/build.gradle.kts +++ b/sentry-android-navigation3/build.gradle.kts @@ -46,6 +46,10 @@ android { warningsAsErrors = true checkDependencies = true + // Lifecycle 2.8.3's lint checks are incompatible with AGP 9.5. Lifecycle 2.10.0 fixes them, + // but raises minSdk to 23 while this SDK still supports 21. + disable += "ObsoleteLintCustomCheck" + // We run a full lint analysis as build part in CI, so skip vital checks for assemble tasks. checkReleaseBuilds = false } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt new file mode 100644 index 00000000000..0810c41f27a --- /dev/null +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt @@ -0,0 +1,52 @@ +package io.sentry.compose.navigation3.lifecycle + +import android.app.Activity +import android.app.Application +import android.app.Application.ActivityLifecycleCallbacks +import android.os.Bundle + +/** + * Watches every Activity of the host application and reports the Activity lifecycle events that + * [NavLeaseCoordinator] acts on. + * + * - [onActivityResumed] is called when any Activity resumes, including Activities without a + * `SentryNavEffect`. + * + * **Lifetime** + * + * The callbacks are registered at most once per watcher and never unregistered. Like + * `defaultNavLeaseCoordinator`, which owns the watcher used in practice, they're meant to live as + * long as the process. + */ +internal class NavActivityWatcher(private val onActivityResumed: (activity: Activity) -> Unit) { + + private var isWatching = false + + /** Starts watching the Activities of [application]. Does nothing if already watching. */ + fun watch(application: Application) { + if (isWatching) { + return + } + isWatching = true + application.registerActivityLifecycleCallbacks(Callbacks()) + } + + private inner class Callbacks : ActivityLifecycleCallbacks { + + override fun onActivityResumed(activity: Activity) { + this@NavActivityWatcher.onActivityResumed(activity) + } + + override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = Unit + + override fun onActivityStarted(activity: Activity) = Unit + + override fun onActivityPaused(activity: Activity) = Unit + + override fun onActivityStopped(activity: Activity) = Unit + + override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = Unit + + override fun onActivityDestroyed(activity: Activity) = Unit + } +} diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt index 492a7c35526..e2791e31415 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt @@ -1,5 +1,6 @@ package io.sentry.compose.navigation3.lifecycle +import android.app.Activity import androidx.lifecycle.Lifecycle import androidx.lifecycle.Lifecycle.Event.ON_DESTROY import androidx.lifecycle.Lifecycle.Event.ON_PAUSE @@ -10,6 +11,7 @@ import androidx.lifecycle.LifecycleEventObserver import androidx.lifecycle.LifecycleOwner import io.sentry.IScope import io.sentry.IScopes +import io.sentry.SentryLevel /** * Manages a lease in order to determine which [NavScopes] instance can bind nav transactions and @@ -53,6 +55,12 @@ internal class NavLeaseCoordinator { */ private var resumeOrder: Long = 0 + /** + * Reports the host application's Activities to this coordinator so that it can detect whether a + * leaseholder is covered by a non-candidate root. + */ + private val activityWatcher = NavActivityWatcher(onActivityResumed = ::onAnyActivityResumed) + /** * Registers a [nav root][rootInfo] as a lease candidate until disposal or lifecycle destruction. * @@ -70,11 +78,13 @@ internal class NavLeaseCoordinator { return null } + rootInfo.activity?.application?.let { activityWatcher.watch(it) } + val initialResumeOrder = if (lifecycleState == Lifecycle.State.RESUMED) ++resumeOrder else 0 val candidate = Candidate(rootInfo, lifecycleState, initialResumeOrder, publisher, scopes) - // Only list the candidate once addObserver() succeeds, so a failure can't leave behind a - // candidate that never observes its nav root's lifecycle. + // Only list the candidate once addObserver() succeeds, so a failure can't leak the host + // Activity. lifecycle.addObserver(candidate) candidates += candidate return candidate @@ -93,15 +103,15 @@ internal class NavLeaseCoordinator { val holder = leaseHolder - // A nav root that isn't resumed doesn't take the lease from a resumed leaseholder, as it isn't - // the one the user is interacting with (e.g., a preloaded ViewPager2 page next to the current - // page). Instead, it acquires the lease when it resumes. - val wouldDisplaceResumedHolder = - holder != null && - holder.state == Lifecycle.State.RESUMED && - candidate.state != Lifecycle.State.RESUMED + // A nav root that isn't resumed while either the leaseholder or its own host Activity is + // resumed isn't the one the user is interacting with (e.g., a preloaded ViewPager2 page next to + // the current page, which may be a page without a SentryNavEffect and so never a leaseholder). + // Instead, it acquires the lease when it resumes. + val isBesideResumedContent = + candidate.state != Lifecycle.State.RESUMED && + (holder?.state == Lifecycle.State.RESUMED || candidate.isHostActivityResumed) - if (candidate.canAcquireLeaseOnFirstUpdate && !wouldDisplaceResumedHolder) { + if (candidate.canAcquireLeaseOnFirstUpdate && !isBesideResumedContent) { transferLeaseTo(candidate, scope) } } @@ -136,19 +146,48 @@ internal class NavLeaseCoordinator { } } + /** + * Releases the lease without handing it off and unpublishes its nav state from [scope]. The + * candidate retains staged state for a later return. Without a scope, state remains published. + * + * Logs [withdrawalReason]. + */ + private fun withdrawLease(scope: IScope?, withdrawalReason: String) { + val holder = leaseHolder ?: return + + leaseHolder = null + scope + ?.options + ?.logger + ?.log( + SentryLevel.DEBUG, + "Withdrew the Nav3 lease from a nav root hosted by %s: %s.", + holder.hostActivity?.javaClass?.name, + withdrawalReason, + ) + if (scope != null && lastPublisher === holder) { + // Clear the last publisher before calling into the scope (see the class's exception policy). + lastPublisher = null + holder.publisher.unpublishFrom(scope) + } + } + /** * The eligible candidate most recently observed to be in a resumed state, other than [excluding]. * * If [resumedOnly] is set to `true`, only candidates that are resumed right now are considered; - * if `false`, started and paused candidates are also included. + * if `false`, started and paused candidates are also included. Candidates hosted by + * [excludingHostedBy] are skipped. */ private fun mostRecentlyResumedCandidate( excluding: Candidate? = null, resumedOnly: Boolean = false, + excludingHostedBy: Activity? = null, ): Candidate? = candidates .filter { it !== excluding && + (excludingHostedBy == null || it.hostActivity !== excludingHostedBy) && it.canReceiveLease && it.state.isAtLeast(Lifecycle.State.STARTED) && it.resumeOrder > 0 && @@ -176,7 +215,9 @@ internal class NavLeaseCoordinator { * Records that [candidate] paused. * * If [candidate] holds the lease when it pauses, hands it to another resumed nav root (e.g., a - * Fragment whose dialog is being dismissed); otherwise the paused nav root keeps the lease. + * Fragment whose dialog is being dismissed), unless that nav root shares [candidate]'s pausing + * host Activity. If none exists, withdraws it when the host Activity remains resumed; otherwise + * the paused nav root keeps the lease. */ private fun pauseCandidate(candidate: Candidate, scope: IScope?) { candidate.pause() @@ -185,8 +226,22 @@ internal class NavLeaseCoordinator { // window or picture-in-picture mode), and any other paused nav root is beneath it. A displaced // nav root will acquire the lease again when it resumes. if (leaseHolder === candidate) { - mostRecentlyResumedCandidate(excluding = candidate, resumedOnly = true)?.let { next -> - handOffLease(next, scope) + // An Activity pauses before its Fragments, so when the host Activity is pausing, its other + // nav roots are still resumed but about to pause too. Handing them the lease would publish + // their nav state, and start a nav transaction, just as the app is backgrounded or covered. + val next = + mostRecentlyResumedCandidate( + excluding = candidate, + resumedOnly = true, + excludingHostedBy = candidate.hostActivity.takeIf { candidate.isHostActivityPaused }, + ) + when { + next != null -> handOffLease(next, scope) + candidate.isHostActivityResumed -> + withdrawLease( + scope, + withdrawalReason = "the nav root paused while its host Activity stayed resumed", + ) } } } @@ -242,6 +297,43 @@ internal class NavLeaseCoordinator { } } + /** + * Whether [activity] owns a candidate's nav root that + * [can receive the lease][Candidate.canReceiveLease]. + * + * A returning Activity root takes the lease on resume and should not trigger withdrawal first. + */ + private fun isActivityNavRoot(activity: Activity): Boolean = candidates.any { + it.canReceiveLease && it.isActivityNavRoot(activity) + } + + /** Withdraws the lease when the resumed [activity] appears to cover a non-resumed nav root. */ + private fun onAnyActivityResumed(activity: Activity) { + // Assume the resumed `activity` is covering the current leaseholder if i) the leaseholder + // itself isn't resumed, ii) `activity` isn't the leaseholder's host Activity, and iii) + // `activity` isn't itself a nav root (see [isActivityNavRoot]). (i) checks the leaseholder's + // own state rather than its host Activity's, as they differ for Fragment nav roots: a Fragment + // leaseholder that isn't resumed isn't what the user is interacting with, even if its host + // Activity is resumed. + val holder = leaseHolder ?: return + val hostActivity = holder.hostActivity ?: return + if ( + hostActivity === activity || + holder.state == Lifecycle.State.RESUMED || + isActivityNavRoot(activity) + ) { + return + } + + holder.withScopeOrNull { scope -> + withdrawLease( + scope, + withdrawalReason = + "${activity.javaClass.name} was resumed while the nav root wasn't resumed", + ) + } + } + /** * A [nav root][rootInfo] eligible to hold the lease managed by [NavLeaseCoordinator]. * @@ -255,6 +347,19 @@ internal class NavLeaseCoordinator { private val scopes: IScopes, ) : NavLeaseRegistration, LifecycleEventObserver { + val hostActivity: Activity? = rootInfo.activity + + /** Whether the host Activity is resumed; false when its lifecycle is unavailable. */ + val isHostActivityResumed: Boolean + get() = hostActivityState == Lifecycle.State.RESUMED + + /** Whether the host Activity isn't resumed; false when its lifecycle is unavailable. */ + val isHostActivityPaused: Boolean + get() = hostActivityState.let { it != null && it != Lifecycle.State.RESUMED } + + private val hostActivityState: Lifecycle.State? + get() = (hostActivity as? LifecycleOwner)?.lifecycle?.currentState + /** Most recent resume order; zero until this candidate first resumes. */ var resumeOrder: Long = initialResumeOrder private set @@ -353,7 +458,7 @@ internal class NavLeaseCoordinator { * Runs [block] with the scope, or with `null` if [IScopes.configureScope] skips its callback. * Does not rerun a callback that throws. */ - private fun withScopeOrNull(block: (IScope?) -> Unit) { + fun withScopeOrNull(block: (IScope?) -> Unit) { var callbackInvoked = false scopes.configureScope { scope -> callbackInvoked = true @@ -364,6 +469,10 @@ internal class NavLeaseCoordinator { block(null) } } + + /** Whether [activity] itself owns this nav root, rather than a Fragment it hosts. */ + fun isActivityNavRoot(activity: Activity): Boolean = + hostActivity === activity && (activity as? LifecycleOwner)?.lifecycle === lifecycle } } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md index c6aa677e7ce..2b496552c93 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md @@ -56,8 +56,9 @@ A displaced nav root remains eligible for lease restoration until it stops (`ON_ root's nav data is reinstated, and so is its nav transaction if unfinished. Otherwise, the nav root gets a new nav transaction (see [below](#returning-to-a-nav-root)). -> † The exception is an incoming nav root that isn't resumed while the leaseholder is, e.g., a -preloaded ViewPager2 Fragment page next to the current one. The user isn't interacting with such a +> † The exception is an incoming nav root that isn't resumed while the leaseholder or the incoming +nav root's host Activity is, e.g., a preloaded ViewPager2 Fragment page next to the current one, +whether or not the current page has a `SentryNavEffect`. The user isn't interacting with such a nav root yet, so it acquires the lease when it resumes instead. The following diagram illustrates how handoff works: @@ -144,6 +145,29 @@ Alternative at rollback point: B disposes before A stops Finish txB if unfinished │ ``` +### Non-candidate roots and lease withdrawal + +An Activity or Fragment without a `SentryNavEffect` (aka, a "non-candidate root") can't acquire the lease. When one replaces the +leaseholder, the coordinator "withdraws" the lease instead, i.e., it unpublishes the leaseholder's nav state without handing off the lease, +leaving the transaction unbound but running until the leaseholder resumes. + +Non-candidate roots never register, so the coordinator infers their presence from either of two signals: + +- another Activity resumes while the leaseholder isn't resumed, unless the resumed Activity is + itself a nav root, which takes the lease when it resumes; or + +- the leaseholder pauses while its host Activity stays resumed (e.g., a ViewPager2 swipe). (Note + that an Activity pauses before its Fragments do when the Activity is backgrounded or covered.) + +**Limitations**: Leaseholders whose host Activity is unknown are never withdrawn. Fragments that cover the leaseholder without pausing it +(e.g., dialogs) go undetected. These two signals are deliberately the only ones: other surfaces that cover a leaseholder without pausing it +(e.g., bottom sheets or Compose dialogs) aren't detected, and shouldn't be by adding signals here. + +**Limitation (before the covering Activity resumes)**: The lease is withdrawn only once the covering Activity resumes. Until then (i.e., +during its `onCreate` and `onStart`), errors and spans the covering Activity produces are still attributed to the leaseholder. For the same +reason, the covering Activity's `ui.load` transaction isn't bound to the scope: `ActivityLifecycleIntegration` tries to bind it when the +Activity is created, while the leaseholder's nav transaction still holds the scope's transaction slot. + ### Returning to a nav root A nav root is "returned to" when its candidate acquires the lease at any time other than on its first back stack update, e.g., when the @@ -167,3 +191,17 @@ gets a transaction on its first back stack update. See [#6454](https://github.co Bringing the app back to the foreground isn't a return either: the leaseholder keeps the lease while the app is in the background (see `stopCandidate`), so it starts no new nav transaction when it resumes. This matches the rest of the SDK, which starts no transaction when an existing Activity comes back to the foreground. + +### Android ordering assumptions + +`NavLeaseCoordinator` relies on the following sequences, which Android doesn't document but AOSP and androidx have historically upheld: + +- An Activity's `onActivityResumed` callbacks run before its `Lifecycle` dispatches + `ON_RESUME` (see `isActivityNavRoot`). + +- An Activity's `Lifecycle` dispatches `ON_PAUSE` before its Fragments' + lifecycles do. androidx's `ReportFragment` and `FragmentActivity` ensure this, not the + platform. + +Going forward, if an order changes, nav data may be attributed to the wrong nav root, or a nav transaction may go unbound, but nothing would +throw. diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt index 59916420c76..bb558f78c38 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt @@ -32,6 +32,7 @@ import io.sentry.SpanStatus * * This class is ***not*** thread-safe. Apply the thread safety policy from [NavScopes]. */ +@Suppress("TooManyFunctions") internal class StagedNavTransaction(private val scopes: IScopes) { /** diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt index c30f296fd39..2d6b11eeda5 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt @@ -41,7 +41,6 @@ import org.mockito.kotlin.mock import org.mockito.kotlin.whenever import org.robolectric.Robolectric import org.robolectric.Shadows.shadowOf -import org.robolectric.android.controller.ActivityController import org.robolectric.annotation.Config /** @@ -58,6 +57,7 @@ import org.robolectric.annotation.Config */ @RunWith(AndroidJUnit4::class) @Config(sdk = [30]) +@Suppress("LargeClass") class SentryNavEffectLifecycleIntegrationTest { @After @@ -399,6 +399,31 @@ class SentryNavEffectLifecycleIntegrationTest { } // ViewPager2 pages + + /** + * All supported API levels, because below API 29 `ComponentActivity` reports that it paused from + * `ReportFragment` rather than from `onActivityPrePaused`, which must still precede its Fragments + * pausing. + */ + @Test + @Config(sdk = [28, 29, 30]) + fun `backgrounding a view pager activity keeps the current page's nav data and transaction`() { + val fixture = Fixture.create() + val activity = Robolectric.buildActivity(ViewPagerActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + activity.pause() + activity.stop() + fixture.idle() + + assertThat(fixture.events).containsAtLeast("ViewPager:ON_PAUSE", "Home:view:ON_PAUSE").inOrder() + assertThat(fixture.scope.screen).isEqualTo("/Home") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Home") + assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) + assertThat(homeTransaction.isFinished).isFalse() + } + @Test fun `a preloaded view pager page does not displace the current page's nav data and transaction`() { val fixture = Fixture.create() @@ -637,10 +662,136 @@ class SentryNavEffectLifecycleIntegrationTest { .inOrder() } - // Screens without an effect + // Nav roots without a SentryNavEffect + + @Test + fun `swiping to a view pager page without an effect clears the outgoing page's nav data and transaction`() { + val fixture = Fixture.create() + val activity = Robolectric.buildActivity(PlainPageViewPagerActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + fixture.events.clear() + + activity.get().viewPager.setCurrentItem(1, false) + fixture.idle() + + assertThat(fixture.events) + .containsExactly("Home:view:ON_PAUSE", "Plain:view:ON_RESUME") + .inOrder() + assertThat(fixture.scope.screen).isNull() + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + assertThat(homeTransaction.isFinished).isFalse() + } @Test - fun `an activity without an effect keeps the outgoing activity's nav data and transaction on the scope`() { + fun `swiping back to a view pager page from one without an effect restores its nav data and transaction`() { + val fixture = Fixture.create() + val activity = Robolectric.buildActivity(PlainPageViewPagerActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + activity.get().viewPager.setCurrentItem(1, false) + fixture.idle() + + activity.get().viewPager.setCurrentItem(0, false) + fixture.idle() + + assertThat(fixture.scope.screen).isEqualTo("/Home") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Home") + assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) + assertThat(homeTransaction.isFinished).isFalse() + } + + @Test + fun `a view pager page preloaded next to a current page without an effect publishes no nav data or transaction`() { + val fixture = Fixture.create() + + Robolectric.buildActivity(PlainInitialPageViewPagerActivity::class.java).setup() + fixture.idle() + + assertThat(fixture.events) + .containsAtLeast("Plain:view:ON_RESUME", "Home:composed(CREATED)", "Home:view:ON_START") + assertThat(fixture.events).doesNotContain("Home:view:ON_RESUME") + assertThat(fixture.scope.screen).isNull() + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `swiping from a view pager page without an effect to a preloaded page publishes its nav data and transaction`() { + val fixture = Fixture.create() + val activity = Robolectric.buildActivity(PlainInitialPageViewPagerActivity::class.java).setup() + fixture.idle() + + activity.get().viewPager.setCurrentItem(0, false) + fixture.idle() + + assertThat(fixture.scope.screen).isEqualTo("/Home") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Home") + assertThat(fixture.scope.transaction?.name).isEqualTo("/Home") + } + + @Test + fun `recreating a view pager activity on a page without an effect publishes no nav data or transaction`() { + val fixture = Fixture.create() + val activity = Robolectric.buildActivity(PlainPageViewPagerActivity::class.java).setup() + fixture.idle() + activity.get().viewPager.setCurrentItem(1, false) + fixture.idle() + + activity.recreate() + fixture.idle() + + check(activity.get().viewPager.currentItem == 1) + assertThat(fixture.scope.screen).isNull() + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `restoring a view pager on a page without an effect after process death publishes no nav data`() { + val savedInstanceState = Bundle() + Fixture.create().run { + val activity = Robolectric.buildActivity(PlainPageViewPagerActivity::class.java).setup() + idle() + activity.get().viewPager.setCurrentItem(1, false) + idle() + activity.pause().stop().saveInstanceState(savedInstanceState).destroy() + idle() + } + + // A new process starts with a new scope and coordinator. + val fixture = Fixture.create() + val activity = + Robolectric.buildActivity(PlainPageViewPagerActivity::class.java).setup(savedInstanceState) + fixture.idle() + + check(activity.get().viewPager.currentItem == 1) + assertThat(fixture.events).contains("Home:composed(STARTED)") + assertThat(fixture.scope.screen).isNull() + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `recreating a view pager activity on a page with an effect publishes that page's nav data and transaction`() { + val fixture = Fixture.create() + val activity = Robolectric.buildActivity(ViewPagerActivity::class.java).setup() + fixture.idle() + activity.get().viewPager.setCurrentItem(1, false) + fixture.idle() + + activity.recreate() + fixture.idle() + + check(activity.get().viewPager.currentItem == 1) + assertThat(fixture.scope.screen).isEqualTo("/Checkout") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Checkout") + assertThat(fixture.scope.transaction?.name).isEqualTo("/Checkout") + } + + @Test + fun `an activity without an effect clears a covered activity's nav data and unbinds its unfinished transaction`() { val fixture = Fixture.create() val home = Robolectric.buildActivity(HomeActivity::class.java).setup() fixture.idle() @@ -655,6 +806,168 @@ class SentryNavEffectLifecycleIntegrationTest { assertThat(fixture.events) .containsAtLeast("Home:ON_PAUSE", "Plain:ON_RESUME", "Home:ON_STOP") .inOrder() + assertThat(fixture.scope.screen).isNull() + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + assertThat(homeTransaction.isFinished).isFalse() + } + + @Test + fun `an activity without an effect clears the nav data and transaction of an activity it only pauses`() { + val fixture = Fixture.create() + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + + assertThat(fixture.events).doesNotContain("Home:ON_STOP") + assertThat(fixture.scope.screen).isNull() + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + assertThat(homeTransaction.isFinished).isFalse() + } + + @Test + fun `going back to an activity covered by one without an effect restores its nav data and unfinished transaction`() { + val fixture = Fixture.create() + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + home.pause() + val plain = Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + home.stop() + fixture.idle() + + plain.pause() + home.restart() + home.resume() + fixture.idle() + plain.stop() + plain.destroy() + fixture.idle() + + assertThat(fixture.scope.screen).isEqualTo("/Home") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Home") + assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) + assertThat(homeTransaction.isFinished).isFalse() + } + + @Test + fun `going back to an activity covered by one without an effect starts a new nav transaction if its own finished`() { + val fixture = Fixture.create() + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + home.pause() + val plain = Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + home.stop() + fixture.idle() + homeTransaction.finish() + + plain.pause() + home.restart() + home.resume() + fixture.idle() + plain.stop() + plain.destroy() + fixture.idle() + + assertThat(fixture.scope.screen).isEqualTo("/Home") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Home") + assertThat(fixture.scope.transaction?.name).isEqualTo("/Home") + assertThat(fixture.scope.transaction).isNotSameInstanceAs(homeTransaction) + assertThat(fixture.scope.transaction?.isFinished).isFalse() + assertThat(fixture.scope.breadcrumbs.map { it.data["to"] }).containsExactly("/Home") + } + + @Test + fun `navigating while covered by an activity without an effect publishes no nav data or transaction`() { + val fixture = Fixture.create() + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + home.pause() + Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + + fixture.backStack("Home") += "Profile" + fixture.idle() + + assertThat(fixture.scope.breadcrumbs.map { it.data["to"] }) + .containsExactly("/Home", "/Profile") + .inOrder() + assertThat(fixture.scope.screen).isNull() + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `going back to an activity that navigated while covered publishes the destination's nav data and transaction`() { + val fixture = Fixture.create() + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + home.pause() + val plain = Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + fixture.backStack("Home") += "Profile" + fixture.idle() + + plain.pause() + home.resume() + fixture.idle() + + assertThat(homeTransaction.isFinished).isTrue() + assertThat(fixture.scope.screen).isEqualTo("/Profile") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Profile", "/Home").inOrder() + assertThat(fixture.scope.transaction?.name).isEqualTo("/Profile") + assertThat(fixture.scope.transaction?.isFinished).isFalse() + } + + @Test + fun `an activity without an effect removes a covered fragment's nav data and transaction from the scope`() { + val fixture = Fixture.create() + val host = Robolectric.buildActivity(FragmentHostActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + host.pause() + Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + host.stop() + fixture.idle() + + assertThat(fixture.events).containsAtLeast("Home:view:ON_PAUSE", "Plain:ON_RESUME").inOrder() + assertThat(fixture.scope.screen).isNull() + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + assertThat(homeTransaction.isFinished).isFalse() + } + + @Test + fun `going back to a fragment covered by an activity without an effect restores its nav data and transaction`() { + val fixture = Fixture.create() + val host = Robolectric.buildActivity(FragmentHostActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + host.pause() + val plain = Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + host.stop() + fixture.idle() + + plain.pause() + host.restart() + host.resume() + fixture.idle() + plain.stop() + plain.destroy() + fixture.idle() + assertThat(fixture.scope.screen).isEqualTo("/Home") assertThat(fixture.scope.navigationEntries()).containsExactly("/Home") assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) @@ -759,10 +1072,13 @@ class SentryNavEffectLifecycleIntegrationTest { } } + /** A [NavEffectActivity] whose SentryNavEffect observes the "Home" back stack. */ internal class HomeActivity : NavEffectActivity("Home") + /** A [NavEffectActivity] whose SentryNavEffect observes the "Checkout" back stack. */ internal class CheckoutActivity : NavEffectActivity("Checkout") + /** An Activity whose content has no SentryNavEffect. */ internal class PlainActivity : FragmentActivity() { override fun onCreate(savedInstanceState: Bundle?) { @@ -772,6 +1088,7 @@ class SentryNavEffectLifecycleIntegrationTest { } } + /** An Activity with no SentryNavEffect of its own, hosting a [HomeFragment] that has one. */ internal class FragmentHostActivity : FragmentActivity() { override fun onCreate(savedInstanceState: Bundle?) { @@ -787,7 +1104,15 @@ class SentryNavEffectLifecycleIntegrationTest { } } - internal class ViewPagerActivity : FragmentActivity() { + /** + * An Activity without a SentryNavEffect of its own whose ViewPager2 pages are a [HomeFragment] + * and, by default, a [CheckoutFragment], each with a SentryNavEffect. The page at [initialPage] + * is current when the Activity is created. + */ + internal open class ViewPagerActivity( + private val createSecondPage: () -> Fragment = ::CheckoutFragment, + private val initialPage: Int = 0, + ) : FragmentActivity() { lateinit var viewPager: ViewPager2 @@ -796,23 +1121,43 @@ class SentryNavEffectLifecycleIntegrationTest { lifecycle.addObserver(Fixture.require().eventRecorder("ViewPager")) viewPager = ViewPager2(this).apply { + // An ID lets the ViewPager2 save and restore its current page and pages. + id = VIEW_PAGER_ID // Preload the page next to the current one. FragmentStateAdapter caps a preloaded page // at STARTED and resumes only the current page. offscreenPageLimit = 1 - adapter = PagesAdapter(this@ViewPagerActivity) + adapter = PagesAdapter(this@ViewPagerActivity, createSecondPage) + setCurrentItem(initialPage, false) } setContentView(viewPager) } - private class PagesAdapter(activity: FragmentActivity) : FragmentStateAdapter(activity) { + private companion object { + const val VIEW_PAGER_ID = 0x7f0b0001 + } + + private class PagesAdapter( + activity: FragmentActivity, + private val createSecondPage: () -> Fragment, + ) : FragmentStateAdapter(activity) { override fun getItemCount(): Int = 2 override fun createFragment(position: Int): Fragment = - if (position == 0) HomeFragment() else CheckoutFragment() + if (position == 0) HomeFragment() else createSecondPage() } } + /** A [ViewPagerActivity] whose second page has no SentryNavEffect. */ + internal class PlainPageViewPagerActivity : ViewPagerActivity(::PlainFragment) + + /** + * Like [PlainPageViewPagerActivity], but its current page is initially the one without a + * SentryNavEffect, e.g., as after rotating the device on that page. + */ + internal class PlainInitialPageViewPagerActivity : + ViewPagerActivity(::PlainFragment, initialPage = 1) + internal open class NavEffectFragment(private val route: String) : Fragment() { override fun onCreateView( @@ -830,6 +1175,7 @@ class SentryNavEffectLifecycleIntegrationTest { internal class CheckoutFragment : NavEffectFragment("Checkout") + /** A Fragment whose content has no SentryNavEffect. */ internal class PlainFragment : Fragment() { override fun onCreateView( @@ -927,6 +1273,8 @@ class SentryNavEffectLifecycleIntegrationTest { PlainActivity::class.java, FragmentHostActivity::class.java, ViewPagerActivity::class.java, + PlainPageViewPagerActivity::class.java, + PlainInitialPageViewPagerActivity::class.java, ) .forEach { activityClass: Class -> packageManager.addActivityIfNotPresent( diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt index 0ce82b832a6..7a0ed005f8e 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt @@ -3,11 +3,13 @@ package io.sentry.compose.navigation3 import android.app.Application import android.content.ComponentName import androidx.activity.ComponentActivity +import androidx.compose.runtime.Composable import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.mutableStateListOf import androidx.compose.runtime.mutableStateOf +import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.test.junit4.createAndroidComposeRule import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleOwner @@ -576,7 +578,7 @@ class SentryNavEffectTest { val owner = NavTestLifecycle() val options = SentryNavOptions() composeRule.setContent { - CompositionLocalProvider(LocalLifecycleOwner provides owner) { + WithoutHostActivity(owner) { SentryNavEffect( backStack = listOf("Home"), backStackEntryMapper = BackStackEntryMapper { SentryBackStackEntry(it) }, @@ -605,7 +607,7 @@ class SentryNavEffectTest { val backStack = mutableStateListOf("Home") val options = SentryNavOptions() composeRule.setContent { - CompositionLocalProvider(LocalLifecycleOwner provides owner) { + WithoutHostActivity(owner) { SentryNavEffect( backStack = backStack, backStackEntryMapper = BackStackEntryMapper { SentryBackStackEntry(it) }, @@ -638,11 +640,11 @@ class SentryNavEffectTest { val options = SentryNavOptions() val mapper = BackStackEntryMapper { SentryBackStackEntry(it) } composeRule.setContent { - CompositionLocalProvider(LocalLifecycleOwner provides ownerA) { + WithoutHostActivity(ownerA) { SentryNavEffect(backStackA, mapper, options, fixture.scopes, fixture.coordinator) } if (showB.value) { - CompositionLocalProvider(LocalLifecycleOwner provides ownerB) { + WithoutHostActivity(ownerB) { SentryNavEffect(listOf("Picker"), mapper, options, fixture.scopes, fixture.coordinator) } } @@ -692,11 +694,11 @@ class SentryNavEffectTest { val options = SentryNavOptions() val mapper = BackStackEntryMapper { SentryBackStackEntry(it) } composeRule.setContent { - CompositionLocalProvider(LocalLifecycleOwner provides beneath) { + WithoutHostActivity(beneath) { SentryNavEffect(listOf("Products"), mapper, options, fixture.scopes, fixture.coordinator) } if (showTranslucent.value) { - CompositionLocalProvider(LocalLifecycleOwner provides translucent) { + WithoutHostActivity(translucent) { SentryNavEffect(listOf("Checkout"), mapper, options, fixture.scopes, fixture.coordinator) } } @@ -734,7 +736,7 @@ class SentryNavEffectTest { val owner = NavTestLifecycle(Lifecycle.State.RESUMED) val options = SentryNavOptions() composeRule.setContent { - CompositionLocalProvider(LocalLifecycleOwner provides owner) { + WithoutHostActivity(owner) { SentryNavEffect( backStack = listOf("Home"), backStackEntryMapper = BackStackEntryMapper { SentryBackStackEntry(it) }, @@ -766,7 +768,7 @@ class SentryNavEffectTest { val owner = mutableStateOf(originalOwner) val options = SentryNavOptions() composeRule.setContent { - CompositionLocalProvider(LocalLifecycleOwner provides owner.value) { + WithoutHostActivity(owner.value) { SentryNavEffect( backStack = listOf("Home"), backStackEntryMapper = BackStackEntryMapper { SentryBackStackEntry(it) }, @@ -794,6 +796,21 @@ class SentryNavEffectTest { } } + /** + * Provides [owner] to [content] without a host Activity. Useful for tests whose owners stand in + * for Activities. Otherwise, the compose rule's Activity, which stays resumed, would be treated + * as the owner's host. Pausing the owner would then withdraw the lease, and an owner that isn't + * resumed yet couldn't acquire it on its first back stack update. + */ + @Composable + private fun WithoutHostActivity(owner: LifecycleOwner, content: @Composable () -> Unit) { + CompositionLocalProvider( + LocalLifecycleOwner provides owner, + LocalContext provides LocalContext.current.applicationContext, + content = content, + ) + } + private fun IScope.navigationBackStack(): List>? { val navigationContext = contexts[NAVIGATION_CONTEXT_KEY] as? Map<*, *> ?: return null diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt index 20acfc4078b..102d59b35d7 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt @@ -1,5 +1,8 @@ package io.sentry.compose.navigation3.lifecycle +import android.app.Activity +import android.app.Application +import android.app.Application.ActivityLifecycleCallbacks import androidx.lifecycle.Lifecycle import androidx.lifecycle.Lifecycle.State.CREATED import androidx.lifecycle.Lifecycle.State.DESTROYED @@ -8,16 +11,20 @@ import androidx.lifecycle.Lifecycle.State.STARTED import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.LifecycleRegistry import com.google.common.truth.Truth.assertThat +import io.sentry.ILogger import io.sentry.IScope import io.sentry.IScopes import io.sentry.Scope import io.sentry.ScopeCallback +import io.sentry.SentryLevel import io.sentry.SentryOptions import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever @Suppress("LargeClass") @@ -54,6 +61,44 @@ class NavLeaseCoordinatorTest { assertThat(second.hasLease).isTrue() } + @Test + fun `register() - registers activity lifecycle callbacks with the host activity's application`() { + val fixture = Fixture.create() + + fixture.register("owner", TestOwner(CREATED), host = fixture.hostActivity) + + assertThat(fixture.registeredActivityCallbacks).hasSize(1) + } + + @Test + fun `register() - registers activity lifecycle callbacks only once for several hosted candidates`() { + val fixture = Fixture.create() + fixture.register("first", TestOwner(CREATED), host = fixture.hostActivity) + + fixture.register("second", TestOwner(CREATED), host = fixture.coveringActivity) + + assertThat(fixture.registeredActivityCallbacks).hasSize(1) + } + + @Test + fun `register() - doesn't register activity lifecycle callbacks without a host activity`() { + val fixture = Fixture.create() + + fixture.register("owner", TestOwner(CREATED)) + + assertThat(fixture.registeredActivityCallbacks).isEmpty() + } + + @Test + fun `register() - registers activity lifecycle callbacks for a later host activity if an earlier one has no app`() { + val fixture = Fixture.create() + fixture.register("detached", TestOwner(CREATED), host = fixture.activity(application = null)) + + fixture.register("owner", TestOwner(CREATED), host = fixture.hostActivity) + + assertThat(fixture.registeredActivityCallbacks).hasSize(1) + } + // endregion // region acquiring the nav lease on the first back stack update @@ -218,6 +263,80 @@ class NavLeaseCoordinatorTest { assertThat(registration.hasLease).isTrue() } + @Test + fun `a non-resumed owner doesn't acquire the nav lease on its first update while its host activity is resumed`() { + val fixture = Fixture.create() + val host = fixture.lifecycleOwnerActivity(TestOwner(RESUMED).lifecycle) + val registration = fixture.register("owner", TestOwner(STARTED), host = host) + + registration.onBackStackUpdate() + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.published).isEmpty() + } + + @Test + fun `an owner kept from the nav lease by its resumed host activity acquires it when it resumes`() { + val fixture = Fixture.create() + val host = fixture.lifecycleOwnerActivity(TestOwner(RESUMED).lifecycle) + val owner = TestOwner(STARTED) + val registration = fixture.acquireLease("owner", owner, host = host) + + owner.lifecycle.currentState = RESUMED + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") + } + + @Test + fun `a non-resumed owner doesn't take the nav lease from a paused leaseholder while its host activity is resumed`() { + val fixture = Fixture.create() + val host = fixture.lifecycleOwnerActivity(TestOwner(RESUMED).lifecycle) + val incumbentOwner = TestOwner(RESUMED) + val incumbent = fixture.acquireLease("incumbent", incumbentOwner) + incumbentOwner.lifecycle.currentState = STARTED + + fixture.acquireLease("owner", TestOwner(STARTED), host = host) + + assertThat(incumbent.hasLease).isTrue() + assertThat(fixture.published).containsExactly("incumbent") + } + + @Test + fun `a non-resumed owner acquires the nav lease on its first update while its host activity isn't resumed`() { + val fixture = Fixture.create() + val host = fixture.lifecycleOwnerActivity(TestOwner(STARTED).lifecycle) + val registration = fixture.register("owner", TestOwner(STARTED), host = host) + + registration.onBackStackUpdate() + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") + } + + @Test + fun `a non-resumed owner acquires the nav lease on its first update if its host activity isn't a lifecycle owner`() { + val fixture = Fixture.create() + val registration = fixture.register("owner", TestOwner(STARTED), host = fixture.hostActivity) + + registration.onBackStackUpdate() + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") + } + + @Test + fun `a resumed owner acquires the nav lease on its first back stack update while its host activity is resumed`() { + val fixture = Fixture.create() + val host = fixture.lifecycleOwnerActivity(TestOwner(RESUMED).lifecycle) + val registration = fixture.register("owner", TestOwner(RESUMED), host = host) + + registration.onBackStackUpdate() + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") + } + @Test fun `acquiring the nav lease unpublishes the previous publisher's nav state before publishing its own`() { val fixture = Fixture.create() @@ -622,6 +741,426 @@ class NavLeaseCoordinatorTest { // endregion + // region lease withdrawal when another Activity covers the leaseholder + + @Test + fun `another activity resuming while the leaseholder is paused withdraws the nav lease`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `another activity resuming while the leaseholder is stopped withdraws the nav lease`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = CREATED + + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `another activity resuming while the leaseholder is resumed keeps the nav lease`() { + val fixture = Fixture.create() + val registration = + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `another activity resuming while the leaseholder is paused logs the withdrawal`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + verify(fixture.logger) + .log( + SentryLevel.DEBUG, + "Withdrew the Nav3 lease from a nav root hosted by %s: %s.", + fixture.hostActivity.javaClass.name, + "${fixture.coveringActivity.javaClass.name} was resumed while the nav root wasn't resumed", + ) + } + + @Test + fun `another activity resuming while the leaseholder is resumed logs nothing`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + verifyNoInteractions(fixture.logger) + } + + @Test + fun `an activity nav root resuming while the leaseholder is paused takes the nav lease without a withdrawal`() { + val fixture = Fixture.create() + val returnedTo = TestOwner(RESUMED) + val returnedToActivity = fixture.lifecycleOwnerActivity(returnedTo.lifecycle) + val returnedToRegistration = + fixture.acquireLease("returnedTo", returnedTo, host = returnedToActivity) + returnedTo.lifecycle.currentState = STARTED + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + returnedTo.lifecycle.currentState = CREATED + owner.lifecycle.currentState = STARTED + + // Android calls back onActivityResumed before dispatching ON_RESUME to the Activity's + // lifecycle. + fixture.activityCallbacks.onActivityResumed(returnedToActivity) + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).containsExactly("returnedTo") + verifyNoInteractions(fixture.logger) + + returnedTo.lifecycle.currentState = RESUMED + + assertThat(returnedToRegistration.hasLease).isTrue() + assertThat(fixture.unpublished).containsExactly("returnedTo", "owner").inOrder() + verifyNoInteractions(fixture.logger) + } + + @Test + fun `an activity nav root that hasn't applied a back stack update resuming withdraws the nav lease`() { + val fixture = Fixture.create() + val returnedTo = TestOwner(CREATED) + val returnedToActivity = fixture.lifecycleOwnerActivity(returnedTo.lifecycle) + fixture.register("returnedTo", returnedTo, host = returnedToActivity) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityResumed(returnedToActivity) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `an activity hosting a fragment nav root resuming while the leaseholder is paused withdraws the nav lease`() { + val fixture = Fixture.create() + val activityOwner = TestOwner(RESUMED) + val hostingActivity = fixture.lifecycleOwnerActivity(activityOwner.lifecycle) + // A Fragment page that isn't resumed applies its first update without acquiring the lease. + fixture.acquireLease("page", TestOwner(STARTED), host = hostingActivity) + activityOwner.lifecycle.currentState = STARTED + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityResumed(hostingActivity) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `the leaseholder's host activity resuming keeps the nav lease`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityResumed(fixture.hostActivity) + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `another activity resuming keeps the nav lease of a leaseholder whose host activity is unknown`() { + val fixture = Fixture.create() + fixture.register("hosted", TestOwner(CREATED), host = fixture.hostActivity) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `another activity resuming without a leaseholder doesn't unpublish any nav state`() { + val fixture = Fixture.create() + val registration = fixture.register("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `withdrawing the nav lease doesn't hand it to another paused owner`() { + val fixture = Fixture.create() + val previousOwner = TestOwner(RESUMED) + val previous = fixture.acquireLease("previous", previousOwner, host = fixture.hostActivity) + previousOwner.lifecycle.currentState = STARTED + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + assertThat(registration.hasLease).isFalse() + assertThat(previous.hasLease).isFalse() + assertThat(fixture.published).containsExactly("previous", "owner").inOrder() + } + + @Test + fun `a withdrawn owner reacquires the nav lease and republishes its nav state when it resumes`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + owner.lifecycle.currentState = RESUMED + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner", "owner") + } + + @Test + fun `an owner acquiring the nav lease after a withdrawal doesn't unpublish the withdrawn owner's nav state again`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + + val covering = + fixture.acquireLease("covering", TestOwner(CREATED), host = fixture.coveringActivity) + + assertThat(covering.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner", "covering").inOrder() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `withdrawing the nav lease without a scope releases it but leaves its nav state published`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.withoutScope { + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + } + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `acquiring the nav lease after a withdrawal without a scope unpublishes the withdrawn owner's nav state`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + fixture.withoutScope { + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + } + + fixture.acquireLease("covering", TestOwner(CREATED), host = fixture.coveringActivity) + + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `if unpublishing throws during a withdrawal, the lease is released and the nav state isn't unpublished again`() { + val fixture = Fixture.create() + val failure = IllegalStateException("unpublish failed") + var unpublishAttempts = 0 + val owner = TestOwner(RESUMED) + val registration = + fixture.acquireLease( + "owner", + owner, + publisher = + fixture.publisher( + "owner", + onUnpublishFrom = { + unpublishAttempts++ + throw failure + }, + ), + host = fixture.hostActivity, + ) + owner.lifecycle.currentState = STARTED + + val thrown = + assertThrows(IllegalStateException::class.java) { + fixture.activityCallbacks.onActivityResumed(fixture.coveringActivity) + } + + assertThat(thrown).isSameInstanceAs(failure) + assertThat(registration.hasLease).isFalse() + + val covering = fixture.acquireLease("covering", TestOwner(CREATED)) + + assertThat(covering.hasLease).isTrue() + assertThat(unpublishAttempts).isEqualTo(1) + } + + // endregion + + // region lease withdrawal when the leaseholder pauses within a resumed Activity + + @Test + fun `pausing the leaseholder while its host activity is resumed withdraws the lease and unpublishes nav state`() { + val fixture = Fixture.create() + val host = fixture.lifecycleOwnerActivity(TestOwner(RESUMED).lifecycle) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = host) + + owner.lifecycle.currentState = STARTED + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `pausing the leaseholder while its host activity is resumed logs the withdrawal`() { + val fixture = Fixture.create() + val host = fixture.lifecycleOwnerActivity(TestOwner(RESUMED).lifecycle) + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = host) + + owner.lifecycle.currentState = STARTED + + verify(fixture.logger) + .log( + SentryLevel.DEBUG, + "Withdrew the Nav3 lease from a nav root hosted by %s: %s.", + host.javaClass.name, + "the nav root paused while its host Activity stayed resumed", + ) + } + + @Test + fun `pausing the leaseholder while its host activity is paused logs nothing`() { + val fixture = Fixture.create() + val hostOwner = TestOwner(RESUMED) + val host = fixture.lifecycleOwnerActivity(hostOwner.lifecycle) + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = host) + hostOwner.lifecycle.currentState = STARTED + + owner.lifecycle.currentState = STARTED + + verifyNoInteractions(fixture.logger) + } + + @Test + fun `pausing the leaseholder while its host activity is paused keeps the nav lease`() { + val fixture = Fixture.create() + val hostOwner = TestOwner(RESUMED) + val host = fixture.lifecycleOwnerActivity(hostOwner.lifecycle) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = host) + hostOwner.lifecycle.currentState = STARTED + + owner.lifecycle.currentState = STARTED + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `pausing a leaseholder that observes its host activity's own lifecycle keeps the nav lease`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val host = fixture.lifecycleOwnerActivity(owner.lifecycle) + val registration = fixture.acquireLease("owner", owner, host = host) + + owner.lifecycle.currentState = STARTED + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `pausing the leaseholder keeps the nav lease if its host activity isn't a lifecycle owner`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + + owner.lifecycle.currentState = STARTED + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `pausing the leaseholder within its resumed host activity hands the nav lease to a resumed owner`() { + val fixture = Fixture.create() + val host = fixture.lifecycleOwnerActivity(TestOwner(RESUMED).lifecycle) + val other = fixture.acquireLease("other", TestOwner(RESUMED), host = host) + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = host) + + owner.lifecycle.currentState = STARTED + + assertThat(other.hasLease).isTrue() + assertThat(fixture.published).containsExactly("other", "owner", "other").inOrder() + } + + @Test + fun `pausing a leaseholder whose host is paused keeps the lease from a resumed owner in the same activity`() { + val fixture = Fixture.create() + val hostOwner = TestOwner(RESUMED) + val host = fixture.lifecycleOwnerActivity(hostOwner.lifecycle) + val other = fixture.acquireLease("other", TestOwner(RESUMED), host = host) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = host) + hostOwner.lifecycle.currentState = STARTED + + owner.lifecycle.currentState = STARTED + + assertThat(registration.hasLease).isTrue() + assertThat(other.hasLease).isFalse() + assertThat(fixture.published).containsExactly("other", "owner").inOrder() + } + + @Test + fun `pausing a leaseholder whose host is paused hands the lease to a resumed owner in another activity`() { + val fixture = Fixture.create() + val otherHost = fixture.lifecycleOwnerActivity(TestOwner(RESUMED).lifecycle) + val other = fixture.acquireLease("other", TestOwner(RESUMED), host = otherHost) + val hostOwner = TestOwner(RESUMED) + val host = fixture.lifecycleOwnerActivity(hostOwner.lifecycle) + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = host) + hostOwner.lifecycle.currentState = STARTED + + owner.lifecycle.currentState = STARTED + + assertThat(other.hasLease).isTrue() + assertThat(fixture.published).containsExactly("other", "owner", "other").inOrder() + } + + // endregion + // region unregistering @Test @@ -860,13 +1399,24 @@ class NavLeaseCoordinatorTest { private class Fixture private constructor() { val navLeaseCoordinator = NavLeaseCoordinator() - val scope = Scope(SentryOptions()) + + val logger = mock() + + // Shared by scope and scopes, as in production. + private val options = + SentryOptions().apply { + setLogger(this@Fixture.logger) + isDebug = true + } + + val scope = Scope(options) /** Whether [scopes] runs `configureScope` callbacks; when false, it skips them. */ private var providesScope = true val scopes = mock().also { scopes -> + whenever(scopes.options).thenReturn(options) doAnswer { if (providesScope) { (it.arguments[0] as ScopeCallback).run(scope) @@ -903,29 +1453,67 @@ class NavLeaseCoordinatorTest { onUnregistered: (IScope?) -> Unit = { unregistered += name to it }, ): INavStatePublisher = FakeNavStatePublisher(onPublishTo, onUnpublishFrom, onUnregistered) + /** The application of [hostActivity] and [coveringActivity]. */ + val application = mock() + + /** Activity lifecycle callbacks registered with [application], in registration order. */ + val registeredActivityCallbacks = mutableListOf() + + /** The activity lifecycle callbacks the coordinator registered with [application]. */ + val activityCallbacks: ActivityLifecycleCallbacks + get() = registeredActivityCallbacks.single() + + /** An Activity of [application] that hosts candidates. */ + val hostActivity: Activity = activity(application) + + /** Another Activity of [application], which can cover [hostActivity]. */ + val coveringActivity: Activity = activity(application) + + init { + doAnswer { + registeredActivityCallbacks += it.arguments[0] as ActivityLifecycleCallbacks + null + } + .whenever(application) + .registerActivityLifecycleCallbacks(any()) + } + + /** An Activity whose application is [application], or that has none given `null`. */ + fun activity(application: Application?): Activity = + mock().also { whenever(it.application).thenReturn(application) } + + /** An Activity of [application] that's a [LifecycleOwner] of [lifecycle]. */ + fun lifecycleOwnerActivity(lifecycle: Lifecycle): Activity = + mock(extraInterfaces = arrayOf(LifecycleOwner::class)).also { + whenever(it.application).thenReturn(application) + whenever((it as LifecycleOwner).lifecycle).thenReturn(lifecycle) + } + /** * Registers a candidate with the publisher named [name] for a nav root whose lifecycle is - * [owner]'s. + * [owner]'s and whose Activity is [host]. */ fun register( name: String, owner: TestOwner, publisher: INavStatePublisher = publisher(name), + host: Activity? = null, ): NavLeaseRegistration { owners += owner - val rootInfo = NavRootInfo(owner.lifecycle, activity = null) + val rootInfo = NavRootInfo(owner.lifecycle, activity = host) return checkNotNull(navLeaseCoordinator.register(scopes, rootInfo, publisher)) } /** - * Registers a candidate for [owner]'s lifecycle and applies its first back stack update, which - * acquires the nav lease if the owner's lifecycle permits it. + * Registers a candidate for [owner]'s lifecycle, hosted by [host], and applies its first back + * stack update, which acquires the nav lease if the owner's lifecycle permits it. */ fun acquireLease( name: String, owner: TestOwner, publisher: INavStatePublisher = publisher(name), - ): NavLeaseRegistration = register(name, owner, publisher).also { it.onBackStackUpdate() } + host: Activity? = null, + ): NavLeaseRegistration = register(name, owner, publisher, host).also { it.onBackStackUpdate() } /** Runs [block] while `configureScope` skips its callbacks, e.g., as if the SDK is disabled. */ fun withoutScope(block: () -> Unit) { diff --git a/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml b/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml index 5b0f8dd715e..3a8c263e0f4 100644 --- a/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml +++ b/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml @@ -147,6 +147,11 @@ android:exported="false" android:theme="@style/AppTheme.Main.Translucent" /> + + Unit) { modifier = Modifier.testTag(nav3TestTag("multi_fragment_current_page")), ) Row(horizontalArrangement = Arrangement.spacedBy(8.dp)) { - Nav3RootFragment.pageRoots.indices.forEach { index -> + (0 until PAGER_PAGE_COUNT).forEach { index -> OutlinedButton( onClick = { onSelectPage(index) }, modifier = Modifier.testTag(nav3TestTag("multi_fragment_page_$index")), @@ -451,5 +454,7 @@ private fun PagerHeader(currentPage: Int, onSelectPage: (Int) -> Unit) { } } +private const val PAGER_PAGE_COUNT = 4 + private const val MATCH_PARENT = ViewGroup.LayoutParams.MATCH_PARENT private const val WRAP_CONTENT = ViewGroup.LayoutParams.WRAP_CONTENT diff --git a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3PlainScreens.kt b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3PlainScreens.kt new file mode 100644 index 00000000000..6d64c7da45f --- /dev/null +++ b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3PlainScreens.kt @@ -0,0 +1,206 @@ +package io.sentry.samples.android.navigation.nav3 + +import android.content.Intent +import android.os.Bundle +import android.util.Log +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import androidx.activity.ComponentActivity +import androidx.activity.compose.setContent +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.safeDrawingPadding +import androidx.compose.material3.Text +import androidx.compose.material3.TextButton +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberCoroutineScope +import androidx.compose.runtime.setValue +import androidx.compose.ui.ExperimentalComposeUiApi +import androidx.compose.ui.Modifier +import androidx.compose.ui.platform.ComposeView +import androidx.compose.ui.platform.LocalContext +import androidx.compose.ui.platform.ViewCompositionStrategy +import androidx.compose.ui.platform.testTag +import androidx.compose.ui.semantics.semantics +import androidx.compose.ui.semantics.testTagsAsResourceId +import androidx.compose.ui.unit.dp +import androidx.fragment.app.Fragment +import io.sentry.ISpan +import io.sentry.Sentry +import io.sentry.samples.android.navigation.common.NavigationSampleConfigSnapshot +import io.sentry.samples.android.navigation.common.applyToCurrentOptions +import io.sentry.samples.android.navigation.common.currentNavigationSampleConfigSnapshot +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch + +/** Intentionally has no NavDisplay or SentryNavEffect; launch normally to stay in the same task. */ +class Nav3PlainActivity : ComponentActivity() { + + override fun onCreate(savedInstanceState: Bundle?) { + super.onCreate(savedInstanceState) + // The system can restore this Activity without recreating the Nav3 Activity that set these. + NavigationSampleConfigSnapshot( + enableScreenTracking = intent.getBooleanExtra(EXTRA_ENABLE_SCREEN_TRACKING, true), + enableUserInteractionTransactions = + intent.getBooleanExtra(EXTRA_ENABLE_USER_INTERACTION_TRANSACTIONS, false), + enableUserInteractionBreadcrumbs = + intent.getBooleanExtra(EXTRA_ENABLE_USER_INTERACTION_BREADCRUMBS, false), + ) + .applyToCurrentOptions() + setContent { + Nav3SampleTheme { + PlainNav3Screen("Plain Activity", onBack = ::finish) + // Let ui.load finish normally rather than wait for the sample's TTFD timeout. + LaunchedEffect(Unit) { + reportFullyDrawn() + Sentry.reportFullyDisplayed() + } + } + } + } +} + +/** A real non-candidate pager page: no navigation integration is composed in this Fragment. */ +class Nav3PlainFragment : Fragment() { + + override fun onCreateView( + inflater: LayoutInflater, + container: ViewGroup?, + savedInstanceState: Bundle?, + ): View = + ComposeView(requireContext()).apply { + setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) + setContent { Nav3SampleTheme { PlainNav3Screen("Plain page (3)") } } + } +} + +@Composable +internal fun Nav3OpenPlainActivityButton(modifier: Modifier = Modifier) { + val context = LocalContext.current + Nav3SentryButton( + label = "Plain Activity", + onClick = { + val options = currentNavigationSampleConfigSnapshot() + context.startActivity( + Intent(context, Nav3PlainActivity::class.java) + .putExtra(EXTRA_ENABLE_SCREEN_TRACKING, options.enableScreenTracking) + .putExtra( + EXTRA_ENABLE_USER_INTERACTION_TRANSACTIONS, + options.enableUserInteractionTransactions, + ) + .putExtra( + EXTRA_ENABLE_USER_INTERACTION_BREADCRUMBS, + options.enableUserInteractionBreadcrumbs, + ) + ) + }, + modifier = modifier, + ) +} + +/** + * A screen without a SentryNavEffect, for checking manually that no nav data or nav transaction + * from a previously shown Nav3 root is attributed to it. + */ +@OptIn(ExperimentalComposeUiApi::class) +@Composable +private fun PlainNav3Screen(title: String, onBack: (() -> Unit)? = null) { + val coroutineScope = rememberCoroutineScope() + var snapshot by remember { mutableStateOf(PlainScreenScopeProbe.snapshot()) } + var result by remember { mutableStateOf("") } + LaunchedEffect(Unit) { + while (true) { + snapshot = PlainScreenScopeProbe.snapshot() + delay(SNAPSHOT_REFRESH_MILLIS) + } + } + Column( + Modifier.fillMaxSize().safeDrawingPadding().padding(16.dp).semantics { + testTagsAsResourceId = true + } + ) { + Text(title, modifier = Modifier.testTag(nav3TestTag("plain_title"))) + Text( + "This screen has no SentryNavEffect. The previous root's Navigation context should be absent." + ) + Text(snapshot, modifier = Modifier.testTag(nav3TestTag("plain_scope_state"))) + Text(result, modifier = Modifier.testTag(nav3TestTag("plain_result"))) + TextButton( + onClick = { coroutineScope.launch { result = PlainScreenScopeProbe.emitSpan() } }, + modifier = Modifier.testTag(nav3TestTag("plain_emit_span")), + ) { + Text("Emit span") + } + TextButton( + onClick = { result = PlainScreenScopeProbe.captureEvent() }, + modifier = Modifier.testTag(nav3TestTag("plain_capture_event")), + ) { + Text("Capture event") + } + if (onBack != null) { + TextButton(onClick = onBack, modifier = Modifier.testTag(nav3TestTag("plain_back"))) { + Text("Back") + } + } + } +} + +/** Reads and exercises the scope from a plain screen. Main-thread only. */ +private object PlainScreenScopeProbe { + + fun snapshot(): String { + var state = "" + Sentry.configureScope { scope -> + val transaction = scope.transaction + state = + "Transaction: ${transaction?.name} (${transaction?.operation})\n" + + "Screen: ${scope.screen}\n" + + "Navigation: ${scope.contexts["Navigation"]}" + } + return state + } + + suspend fun emitSpan(): String { + val parent = Sentry.getSpan() + val span = parent?.startChild("test.navigation.manual", "Nav3 plain screen span") + // The span must finish even if the screen disappears during the delay. + try { + delay(EMITTED_SPAN_DURATION_MILLIS) + } finally { + span?.finish() + } + return record("Emitted span; parent=${parent?.spanContext?.spanId}; ${traceUrl(parent)}") + } + + fun captureEvent(): String { + val id = Sentry.captureMessage("Nav3 plain screen event") + return record("Event: ${sentryEventSearchUrl(dsn, id.toString()) ?: id}") + } + + private fun traceUrl(transaction: ISpan?): String { + val traceId = transaction?.spanContext?.traceId?.toString() ?: return "No parent transaction" + return sentryTraceUrl(dsn, traceId) ?: "Trace: $traceId" + } + + private val dsn: String? + get() = Sentry.getCurrentScopes().options.dsn + + private fun record(message: String): String { + Log.i("Nav3PlainScreen", "$message\n${snapshot()}") + return message + } +} + +private const val SNAPSHOT_REFRESH_MILLIS = 250L +private const val EMITTED_SPAN_DURATION_MILLIS = 300L +private const val EXTRA_ENABLE_SCREEN_TRACKING = "nav3_plain.enable_screen_tracking" +private const val EXTRA_ENABLE_USER_INTERACTION_TRANSACTIONS = + "nav3_plain.enable_user_interaction_transactions" +private const val EXTRA_ENABLE_USER_INTERACTION_BREADCRUMBS = + "nav3_plain.enable_user_interaction_breadcrumbs" diff --git a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3RouteScreens.kt b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3RouteScreens.kt index a67ed6d9d4e..92a7cbbd6bb 100644 --- a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3RouteScreens.kt +++ b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3RouteScreens.kt @@ -217,6 +217,7 @@ internal fun SentryControls( onClick = onCaptureException, modifier = Modifier.weight(1f), ) + Nav3OpenPlainActivityButton(modifier = Modifier.weight(1f)) Nav3SentryButton( label = "Crash App", onClick = onCrashApp, diff --git a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3Routes.kt b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3Routes.kt index 70b5232c533..8dc18c45941 100644 --- a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3Routes.kt +++ b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3Routes.kt @@ -56,8 +56,8 @@ internal fun Nav3Route.routeSpec(): RouteSpec = routeName = Nav3Route.ViewPager2.routeName, title = "ViewPager2", description = - "Each page is a Fragment with its own SentryNavEffect and NavDisplay. Neighboring " + - "pages are preloaded at STARTED. Back returns to the Products Fragment underneath.", + "Pages 0–2 each have a SentryNavEffect and NavDisplay. Page 3 has no navigation " + + "integration. Neighbors are preloaded at STARTED. Back returns to Products.", ) Nav3Route.DeepLink -> RouteSpec( diff --git a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3TransactionHistory.kt b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3TransactionHistory.kt index 21d99a50b2c..c443c9a8bd5 100644 --- a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3TransactionHistory.kt +++ b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3TransactionHistory.kt @@ -187,6 +187,22 @@ private fun sentryTransactionUrl( "×tamp=${timestampSeconds.toLong()}" } +/** The URL of trace [traceId] in the project [dsn] reports to, or `null` if `dsn` names none. */ +internal fun sentryTraceUrl(dsn: String?, traceId: String): String? { + val projectId = dsn?.projectIdFromDsn() ?: return null + return "https://$SENTRY_SAMPLE_ORG_SLUG.sentry.io/explore/traces/trace/$traceId/" + + "?project=$projectId" +} + +/** + * The URL of an issue search for event [eventId] in the project [dsn] reports to, or `null` if + * `dsn` names none. + */ +internal fun sentryEventSearchUrl(dsn: String?, eventId: String): String? { + val projectId = dsn?.projectIdFromDsn() ?: return null + return "https://$SENTRY_SAMPLE_ORG_SLUG.sentry.io/issues/?project=$projectId&query=$eventId" +} + private fun String.projectIdFromDsn(): String? = substringBefore('?').substringBefore('#').trimEnd('/').substringAfterLast('/').takeIf { projectId -> From 81263445d9835a10708d0b5fb200d73b9dd885d6 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Fri, 13 Nov 2026 11:05:39 +0100 Subject: [PATCH 5/5] feat(android): [Part 4] Prefer Activity ui.load transactions over Nav3 transactions (#6484) The Nav3 lease is now also withdrawn when an Activity is created over the leaseholder, not only when it resumes. On API 29+, creation is reported from onActivityPreCreated, before ActivityLifecycleIntegration binds the new Activity's ui.load transaction, so the leaseholder's nav transaction no longer holds the scope's transaction slot and ui.load binds, as with Nav2. Errors and spans from the new Activity's onCreate and onStart are no longer attributed to the leaseholder on any API level. Because ui.load now binds, returning quickly to a nav root while the closed Activity's ui.load is still running would leave the nav root's transaction blocked. The coordinator therefore retries binding the leaseholder's nav transaction once when another Activity is destroyed, within 10s of the blocked attempt. Below API 29, there's no onActivityPreCreated, so the new Activity's ui.load stays unbound as before; this is documented in the package README. --- CHANGELOG.md | 1 + sentry-android-navigation3/build.gradle.kts | 2 + .../lifecycle/NavActivityWatcher.kt | 75 +++- .../lifecycle/NavLeaseCoordinator.kt | 84 ++-- .../navigation3/lifecycle/NavScopes.kt | 11 +- .../compose/navigation3/lifecycle/README.md | 56 ++- .../lifecycle/StagedNavTransaction.kt | 102 ++++- .../CoordinatedBackStackObserverTest.kt | 2 + ...SentryNavEffectLifecycleIntegrationTest.kt | 388 +++++++++++++++++- .../lifecycle/NavLeaseCoordinatorTest.kt | 134 +++++- .../lifecycle/StagedNavTransactionTest.kt | 273 +++++++++++- 11 files changed, 1072 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57a00e38a08..36a50a484a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ ### Behavioral Changes - Nav3: Returning to an Activity or Fragment with its own `SentryNavEffect` within the app (e.g., closing the Activity that covered it, swiping back to its ViewPager2 page, or dismissing a dialog over it) now starts a new navigation transaction for the current destination if the previous one finished, so spans produced upon return can now be consistently captured ([#6482](https://github.com/getsentry/sentry-java/pull/6482)) +- Nav3: `ui.load` transactions are bound to `IScope` in preference to Nav3 transactions when transitioning between Activities, matching Nav2 behavior (API 29+) ([#6484](https://github.com/getsentry/sentry-java/pull/6484)) ### Fixes diff --git a/sentry-android-navigation3/build.gradle.kts b/sentry-android-navigation3/build.gradle.kts index 6e8814f8641..98130d3c3bd 100644 --- a/sentry-android-navigation3/build.gradle.kts +++ b/sentry-android-navigation3/build.gradle.kts @@ -73,6 +73,8 @@ dependencies { compileOnly(libs.androidx.compose.runtime) compileOnly(libs.androidx.compose.ui) + testImplementation(projects.sentryAndroidCore) + testImplementation(projects.sentryTestSupport) testImplementation(libs.androidx.compose.ui.test.junit4) testImplementation(libs.androidx.fragment.ktx) testImplementation(libs.androidx.test.core) diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt index 0810c41f27a..3c949384f57 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt @@ -3,14 +3,39 @@ package io.sentry.compose.navigation3.lifecycle import android.app.Activity import android.app.Application import android.app.Application.ActivityLifecycleCallbacks +import android.os.Build import android.os.Bundle /** * Watches every Activity of the host application and reports the Activity lifecycle events that - * [NavLeaseCoordinator] acts on. + * [NavLeaseCoordinator] acts on, so that the coordinator needn't know how they differ across API + * levels. * - * - [onActivityResumed] is called when any Activity resumes, including Activities without a - * `SentryNavEffect`. + * Each event is reported relative to `ActivityLifecycleIntegration`, which binds an Activity's + * `ui.load` transaction to the scope when the Activity is created and removes it when the Activity + * is destroyed: + * + * - [onActivityCreatedOrResumed] is called when an Activity is created and again when it resumes, + * with [ActivityEvent.CREATED] or [ActivityEvent.RESUMED] as its event. On API 29+, creation is + * reported before `ActivityLifecycleIntegration` tries to bind the Activity's `ui.load` + * transaction; below API 29, after. So below API 29, a withdrawal on creation comes too late for + * that `ui.load` transaction, which stays unbound (see "Non-candidate roots and lease withdrawal" + * in the package README). + * + * - [onActivityDestroyed] is called when an Activity is destroyed, after + * `ActivityLifecycleIntegration` has removed the Activity's `ui.load` transaction from the scope. + * + * **Android ordering assumptions** + * + * The watcher relies on the following callback orders, which Android doesn't document but AOSP has + * historically upheld: + * + * - `Application` calls `ActivityLifecycleCallbacks` in registration order. Below API 29, + * `ActivityLifecycleIntegration`'s callbacks run before this class's, provided the SDK is + * initialized before this class starts [watching][watch]. + * + * - On API 29+, every `onActivityPreCreated` callback runs before any `onActivityCreated` callback, + * and every `onActivityDestroyed` callback before any `onActivityPostDestroyed` callback. * * **Lifetime** * @@ -18,7 +43,19 @@ import android.os.Bundle * `defaultNavLeaseCoordinator`, which owns the watcher used in practice, they're meant to live as * long as the process. */ -internal class NavActivityWatcher(private val onActivityResumed: (activity: Activity) -> Unit) { +internal class NavActivityWatcher( + private val onActivityCreatedOrResumed: (activity: Activity, event: ActivityEvent) -> Unit, + private val onActivityDestroyed: (activity: Activity) -> Unit, +) { + + /** + * The Activity lifecycle event passed to [onActivityCreatedOrResumed], described by [description] + * in past tense (e.g., "created") for log messages. + */ + enum class ActivityEvent(val description: String) { + CREATED("created"), + RESUMED("resumed"), + } private var isWatching = false @@ -33,11 +70,23 @@ internal class NavActivityWatcher(private val onActivityResumed: (activity: Acti private inner class Callbacks : ActivityLifecycleCallbacks { - override fun onActivityResumed(activity: Activity) { - this@NavActivityWatcher.onActivityResumed(activity) + // Only called on API 29+, before any callback's onActivityCreated. + override fun onActivityPreCreated(activity: Activity, savedInstanceState: Bundle?) { + onActivityCreatedOrResumed(activity, ActivityEvent.CREATED) + } + + // Covers API levels below 29, as they don't call onActivityPreCreated. Runs after + // ActivityLifecycleIntegration's onActivityCreated, which finds the slot taken and never binds + // the Activity's ui.load transaction. This is a known limitation (see the package README). + override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { + onActivityCreatedOrResumed(activity, ActivityEvent.CREATED) + } } - override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = Unit + override fun onActivityResumed(activity: Activity) { + onActivityCreatedOrResumed(activity, ActivityEvent.RESUMED) + } override fun onActivityStarted(activity: Activity) = Unit @@ -47,6 +96,16 @@ internal class NavActivityWatcher(private val onActivityResumed: (activity: Acti override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = Unit - override fun onActivityDestroyed(activity: Activity) = Unit + // Covers API levels below 29, which don't call onActivityPostDestroyed. + override fun onActivityDestroyed(activity: Activity) { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { + this@NavActivityWatcher.onActivityDestroyed(activity) + } + } + + // Only called on API 29+, after every callback's onActivityDestroyed. + override fun onActivityPostDestroyed(activity: Activity) { + this@NavActivityWatcher.onActivityDestroyed(activity) + } } } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt index e2791e31415..2049b07ecce 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt @@ -12,6 +12,7 @@ import androidx.lifecycle.LifecycleOwner import io.sentry.IScope import io.sentry.IScopes import io.sentry.SentryLevel +import java.lang.ref.WeakReference /** * Manages a lease in order to determine which [NavScopes] instance can bind nav transactions and @@ -57,9 +58,14 @@ internal class NavLeaseCoordinator { /** * Reports the host application's Activities to this coordinator so that it can detect whether a - * leaseholder is covered by a non-candidate root. + * leaseholder is covered by a non-candidate root, and retry a nav transaction blocked by a closed + * Activity's `ui.load` transaction. */ - private val activityWatcher = NavActivityWatcher(onActivityResumed = ::onAnyActivityResumed) + private val activityWatcher = + NavActivityWatcher( + onActivityCreatedOrResumed = ::onActivityCreatedOrResumed, + onActivityDestroyed = ::onAnyActivityDestroyed, + ) /** * Registers a [nav root][rootInfo] as a lease candidate until disposal or lifecycle destruction. @@ -83,8 +89,8 @@ internal class NavLeaseCoordinator { val initialResumeOrder = if (lifecycleState == Lifecycle.State.RESUMED) ++resumeOrder else 0 val candidate = Candidate(rootInfo, lifecycleState, initialResumeOrder, publisher, scopes) - // Only list the candidate once addObserver() succeeds, so a failure can't leak the host - // Activity. + // Only list the candidate once addObserver() succeeds, so a failure can't leave behind a + // candidate that never observes its nav root's lifecycle. lifecycle.addObserver(candidate) candidates += candidate return candidate @@ -175,13 +181,12 @@ internal class NavLeaseCoordinator { /** * The eligible candidate most recently observed to be in a resumed state, other than [excluding]. * - * If [resumedOnly] is set to `true`, only candidates that are resumed right now are considered; - * if `false`, started and paused candidates are also included. Candidates hosted by - * [excludingHostedBy] are skipped. + * Only candidates whose current state is at least [minimumState] are considered. Candidates + * hosted by [excludingHostedBy] are skipped. */ private fun mostRecentlyResumedCandidate( excluding: Candidate? = null, - resumedOnly: Boolean = false, + minimumState: Lifecycle.State = Lifecycle.State.STARTED, excludingHostedBy: Activity? = null, ): Candidate? = candidates @@ -189,9 +194,8 @@ internal class NavLeaseCoordinator { it !== excluding && (excludingHostedBy == null || it.hostActivity !== excludingHostedBy) && it.canReceiveLease && - it.state.isAtLeast(Lifecycle.State.STARTED) && - it.resumeOrder > 0 && - (!resumedOnly || it.state == Lifecycle.State.RESUMED) + it.state.isAtLeast(minimumState) && + it.resumeOrder > 0 } .maxByOrNull { it.resumeOrder } @@ -232,7 +236,7 @@ internal class NavLeaseCoordinator { val next = mostRecentlyResumedCandidate( excluding = candidate, - resumedOnly = true, + minimumState = Lifecycle.State.RESUMED, excludingHostedBy = candidate.hostActivity.takeIf { candidate.isHostActivityPaused }, ) when { @@ -260,7 +264,9 @@ internal class NavLeaseCoordinator { // beneath it), and that nav root isn't what the user last saw. A paused nav root that comes // back to the foreground will acquire the lease when it resumes. if (leaseHolder === candidate) { - mostRecentlyResumedCandidate(resumedOnly = true)?.let { next -> handOffLease(next, scope) } + mostRecentlyResumedCandidate(minimumState = Lifecycle.State.RESUMED)?.let { next -> + handOffLease(next, scope) + } } } @@ -307,14 +313,17 @@ internal class NavLeaseCoordinator { it.canReceiveLease && it.isActivityNavRoot(activity) } - /** Withdraws the lease when the resumed [activity] appears to cover a non-resumed nav root. */ - private fun onAnyActivityResumed(activity: Activity) { - // Assume the resumed `activity` is covering the current leaseholder if i) the leaseholder - // itself isn't resumed, ii) `activity` isn't the leaseholder's host Activity, and iii) - // `activity` isn't itself a nav root (see [isActivityNavRoot]). (i) checks the leaseholder's - // own state rather than its host Activity's, as they differ for Fragment nav roots: a Fragment - // leaseholder that isn't resumed isn't what the user is interacting with, even if its host - // Activity is resumed. + /** Withdraws the lease when [activity] appears to cover a non-resumed nav root. */ + private fun onActivityCreatedOrResumed( + activity: Activity, + event: NavActivityWatcher.ActivityEvent, + ) { + // Assume the created or resumed `activity` is covering the current leaseholder if i) the + // leaseholder itself isn't resumed, ii) `activity` isn't the leaseholder's host Activity, and + // iii) `activity` isn't itself a nav root (see [isActivityNavRoot]). (i) checks the + // leaseholder's own state rather than its host Activity's, as they differ for Fragment nav + // roots: a Fragment leaseholder that isn't resumed isn't what the user is interacting with, + // even if its host Activity is resumed. val holder = leaseHolder ?: return val hostActivity = holder.hostActivity ?: return if ( @@ -329,11 +338,28 @@ internal class NavLeaseCoordinator { withdrawLease( scope, withdrawalReason = - "${activity.javaClass.name} was resumed while the nav root wasn't resumed", + "${activity.javaClass.name} was ${event.description} while the nav root wasn't resumed", ) } } + /** + * After [activity] is destroyed, asks the leaseholder to retry publication if an Activity's + * `ui.load` blocked it. The publisher decides whether the blocker has left and retry is allowed. + */ + private fun onAnyActivityDestroyed(activity: Activity) { + val holder = leaseHolder ?: return + if (holder.hostActivity === activity) { + return + } + + holder.withScopeOrNull { scope -> + if (scope != null) { + holder.publisher.retryTransactionPublication(scope) + } + } + } + /** * A [nav root][rootInfo] eligible to hold the lease managed by [NavLeaseCoordinator]. * @@ -347,7 +373,13 @@ internal class NavLeaseCoordinator { private val scopes: IScopes, ) : NavLeaseRegistration, LifecycleEventObserver { - val hostActivity: Activity? = rootInfo.activity + // Held weakly because candidates live in a process-wide coordinator: if disposal fails before + // the candidate is removed, it must not keep its destroyed host Activity alive. + private val hostActivityRef: WeakReference? = rootInfo.activity?.let(::WeakReference) + + /** The Activity hosting this nav root, or `null` if it's unknown or has been collected. */ + val hostActivity: Activity? + get() = hostActivityRef?.get() /** Whether the host Activity is resumed; false when its lifecycle is unavailable. */ val isHostActivityResumed: Boolean @@ -502,6 +534,12 @@ internal interface INavStatePublisher { /** Publishes the candidate's staged nav state to [scope]. */ fun publishTo(scope: IScope) + /** + * Retries a nav transaction blocked by an Activity's `ui.load`, subject to + * [StagedNavTransaction.retryPublishIfBlocked]. Leaves other published nav state alone. + */ + fun retryTransactionPublication(scope: IScope) + /** Removes published nav state from [scope], retaining it for a later lease acquisition. */ fun unpublishFrom(scope: IScope) diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt index 39568fa2a29..b8115cacafe 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt @@ -76,7 +76,8 @@ internal class NavScopes( /** This instance's registration with the [coordinator], while it's attached. */ private var registration: NavLeaseRegistration? = null - private val stagedNavTransaction = StagedNavTransaction(delegate) + private val stagedNavTransaction = + StagedNavTransaction(delegate, delegate.options.monotonicTicker) private val stagedNavData = StagedNavData() private val navStatePublisher = NavStatePublisher() @@ -154,6 +155,10 @@ internal class NavScopes( stagedNavTransaction.publishTo(scope) } + override fun retryTransactionPublication(scope: IScope) { + stagedNavTransaction.retryPublishIfBlocked(scope) + } + override fun unpublishFrom(scope: IScope) { stagedNavTransaction.unpublishFrom(scope) stagedNavData.clearPublishedStateFrom(scope) @@ -228,7 +233,9 @@ internal class NavScopes( override fun startNavTransaction(request: NavTransactionRequest) { stagedNavTransaction.stage(request) if (hasLease) { - stagedNavTransaction.publishTo(delegate) + // Don't retry b/c ui.load retry is only for returns to a nav root, not for navigation + // within it. + stagedNavTransaction.publishTo(delegate, canRetry = false) } else { stagedNavData.deferPropagationContextRotation() } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md index 2b496552c93..9670faa20f8 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md @@ -39,17 +39,14 @@ While nav roots overlap (e.g., during an Activity transition), the nav transacti 2. the incoming nav root that may crash before it becomes visible, but not both. `NavLeaseCoordinator` opts for (2) so that the incoming destination populates crash context and any work performed is -associated with it† – the idea being that start-up logic is more likely to cause a crash or produce interesting spans than an outgoing -destination's steady state. - -> † Associating work with the incoming transaction means spans produced by the outgoing nav root -may appear as if they were produced by the incoming nav root (or vice versa, were our policy -reversed). +associated with it – the idea being that start-up logic is more likely to cause a crash or produce interesting spans than an outgoing +destination's steady state. As a consequence, spans produced by the outgoing nav root may appear as if they were produced by the incoming +nav root (or vice versa, were our policy reversed). ### Lease handoff policy The lease is acquired preemptively by an incoming nav root's `NavScopes` instance when it applies its first back stack update -(`onBackStackUpdate`).† +(`onBackStackUpdate`)†. A displaced nav root remains eligible for lease restoration until it stops (`ON_STOP`). It gets the lease back when it resumes (`ON_RESUME`), or sooner if the new nav root leaves or pauses while the displaced nav root is still visible. If restored, the displaced nav @@ -153,8 +150,8 @@ leaving the transaction unbound but running until the leaseholder resumes. Non-candidate roots never register, so the coordinator infers their presence from either of two signals: -- another Activity resumes while the leaseholder isn't resumed, unless the resumed Activity is - itself a nav root, which takes the lease when it resumes; or +- another Activity is created or resumed while the leaseholder isn't resumed, unless the resumed + Activity is itself a nav root, which takes the lease when it resumes; or - the leaseholder pauses while its host Activity stays resumed (e.g., a ViewPager2 swipe). (Note that an Activity pauses before its Fragments do when the Activity is backgrounded or covered.) @@ -163,10 +160,19 @@ Non-candidate roots never register, so the coordinator infers their presence fro (e.g., dialogs) go undetected. These two signals are deliberately the only ones: other surfaces that cover a leaseholder without pausing it (e.g., bottom sheets or Compose dialogs) aren't detected, and shouldn't be by adding signals here. -**Limitation (before the covering Activity resumes)**: The lease is withdrawn only once the covering Activity resumes. Until then (i.e., -during its `onCreate` and `onStart`), errors and spans the covering Activity produces are still attributed to the leaseholder. For the same -reason, the covering Activity's `ui.load` transaction isn't bound to the scope: `ActivityLifecycleIntegration` tries to bind it when the -Activity is created, while the leaseholder's nav transaction still holds the scope's transaction slot. +**Limitation (below API 29)**: An Activity created over the leaseholder doesn't get its `ui.load` transaction bound to the scope. There's no +`onActivityPreCreated` below API 29, so the lease is withdrawn only after `ActivityLifecycleIntegration` has tried to bind the Activity's +`ui.load` transaction and found the leaseholder's nav transaction in the scope's transaction slot. The integration doesn't try again, so its +`ui.load` transaction stays unbound, as it did before withdrawal existed. Withdrawal still keeps the Activity's errors and spans from being +attributed to the leaseholder: spans the Activity produces attach to no transaction until one is bound (e.g., the nav transaction for the +first destination of the Activity's own `SentryNavEffect`, if it has one). Closing this gap would take a change to +`ActivityLifecycleIntegration` that affects every app, so it's deliberately left open. + +As a result, an Activity with its own `SentryNavEffect` gets a different transaction bound for its spans depending on the API level. On +API 29+, its `ui.load` transaction is bound, and its first destination's nav transaction isn't started because the slot is taken. Below +API 29, when the leaseholder's nav transaction held the slot as the Activity was created, its first destination's nav transaction is bound +instead. Either way, `ActivityLifecycleIntegration` still sends the `ui.load` transaction, including its TTID and TTFD spans. Only the +parent of auto-instrumented spans (e.g., HTTP or database spans) differs. ### Returning to a nav root @@ -185,6 +191,24 @@ Either way, the transaction is bound only if the scope's transaction slot is fre happen when the candidate publishes (`INavStatePublisher.publishTo`) its nav state, so the coordinator needn't tell a return from a first acquisition: on its first back stack update, a candidate has no destination to start a transaction for yet. +When the user returns by closing an Activity, that Activity's `ui.load` transaction may still be running and hold the slot. It normally +leaves the slot when the closed Activity is destroyed, which happens after the nav root has resumed. If the Activity is destroyed within +`UI_LOAD_RETRY_WINDOW_SECONDS` of the blocked attempt, binding the nav root's transaction is retried once (see `onAnyActivityDestroyed`). +Otherwise, the nav root goes without a bound nav transaction until its next navigation or return. + +Only the current leaseholder is asked to retry when an Activity is destroyed. Its publisher checks whether the blocked `ui.load` has left +the slot and whether the retry window is still open. A leaseholder that loses or withdraws its lease forgets the blocked publication. + +The retry has three gaps: + +- A `ui.action` transaction started by the gesture that caused the return (e.g., a ViewPager2 swipe or an in-app back button) can block the + nav transaction. Only Activity `ui.load` transactions trigger this retry. +- Spans produced between the nav root's resume and the closed Activity's destruction may attach to the closed Activity's `ui.load`. +- An Activity left without being destroyed (e.g., when another Activity is reordered to the front) triggers no retry. + +Unbinding a finishing Activity's `ui.load` when it pauses would close the last two gaps, but would change `ActivityLifecycleIntegration` +for every app. Giving navigation priority over `ui.action` is an SDK-wide scope-slot policy decision shared with other integrations. + A nav root that's destroyed and recreated after losing the lease (e.g., on a configuration change) isn't returned to: it registers anew and gets a transaction on its first back stack update. See [#6454](https://github.com/getsentry/sentry-java/issues/6454). @@ -194,7 +218,8 @@ existing Activity comes back to the foreground. ### Android ordering assumptions -`NavLeaseCoordinator` relies on the following sequences, which Android doesn't document but AOSP and androidx have historically upheld: +Besides the callback ordering `NavActivityWatcher` relies on, `NavLeaseCoordinator` relies on the following sequences, which Android doesn't +document but AOSP and androidx have historically upheld: - An Activity's `onActivityResumed` callbacks run before its `Lifecycle` dispatches `ON_RESUME` (see `isActivityNavRoot`). @@ -203,5 +228,8 @@ existing Activity comes back to the foreground. lifecycles do. androidx's `ReportFragment` and `FragmentActivity` ensure this, not the platform. +- A visible Activity that finishes is destroyed once the Activity that replaces it reports idle, + or when the system's idle timeout fires (see `UI_LOAD_RETRY_WINDOW_SECONDS`). + Going forward, if an order changes, nav data may be attributed to the wrong nav root, or a nav transaction may go unbound, but nothing would throw. diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt index bb558f78c38..d8ec37f45e3 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt @@ -5,6 +5,27 @@ import io.sentry.IScopes import io.sentry.ITransaction import io.sentry.SentryLevel import io.sentry.SpanStatus +import io.sentry.time.Deadline +import io.sentry.time.MonotonicTicker +import java.lang.ref.WeakReference +import java.util.concurrent.TimeUnit + +/** + * The origin of the `ui.load` transactions `ActivityLifecycleIntegration` starts for Activities. + * + * Mirrors `ActivityLifecycleIntegration.TRACE_ORIGIN`, which isn't public. + */ +internal const val ACTIVITY_UI_LOAD_ORIGIN: String = "auto.ui.activity" + +/** + * Maximum time to retry a nav transaction if an Activity’s `ui.load` transaction blocks its + * publication. + * + * Android usually destroys a closed Activity once the returned-to Activity goes idle, removing its + * `ui.load` transaction from the shared [IScope]. A busy main thread can delay destruction past + * this window; in that case, the return gets no nav transaction until the next navigation. + */ +internal const val UI_LOAD_RETRY_WINDOW_SECONDS: Long = 10 /** * Holds a nav transaction staged for publication to a shared [IScope]. @@ -26,14 +47,18 @@ import io.sentry.SpanStatus * * Navigation doesn't displace an unfinished transaction already bound to the shared `IScope`. This * includes `ui.load`, `ui.action`, and host-app transactions. Changing their relative priority - * requires an SDK-wide policy. + * requires an SDK-wide policy; this class only retries briefly after a closed Activity's `ui.load` + * releases the slot (see [retryPublishIfBlocked]). * * **Thread safety** * * This class is ***not*** thread-safe. Apply the thread safety policy from [NavScopes]. */ @Suppress("TooManyFunctions") -internal class StagedNavTransaction(private val scopes: IScopes) { +internal class StagedNavTransaction( + private val scopes: IScopes, + private val ticker: MonotonicTicker, +) { /** * The request most recently passed to [stage], whatever became of its transaction. Cleared when @@ -48,6 +73,15 @@ internal class StagedNavTransaction(private val scopes: IScopes) { /** The transaction most recently started for [request], unless it has been forgotten since. */ private var transaction: ITransaction? = null + /** + * The most recent [publication][publishTo], if an Activity `ui.load` transaction kept it from + * binding a nav transaction. Cleared when the publication is retried, and when the nav + * transaction is next published, unpublished, finished, or forgotten. + * + * See [retryPublishIfBlocked]. + */ + private var blockedPublication: BlockedPublication? = null + /** * Stages [request], so that the next [publishTo] starts a transaction for it. * @@ -74,25 +108,57 @@ internal class StagedNavTransaction(private val scopes: IScopes) { * Publishes the staged transaction by binding it to [scope], or starts a transaction from the * staged request if needed. An occupied scope slot leaves the transaction unbound and the request * available for later publication. + * + * If [canRetry] is true and an Activity's `ui.load` transaction occupies the scope slot, the + * publication can be retried once the `ui.load` transaction leaves the scope (see + * [retryPublishIfBlocked]). */ - fun publishTo(scope: IScope) { + fun publishTo(scope: IScope, canRetry: Boolean = true) { + blockedPublication = null val request = request ?: return val started = transaction if (started != null && !started.isFinished) { if (scope.isFreeFor(request.name, started)) { bind(scope, started) + } else if (canRetry) { + blockedPublication = scope.blockedPublicationFor(started) } return } transaction = null started?.let { unbind(scope, it) } - startAndBind(scope, request) + // A start declined by the scopes (i.e., a no-op transaction) leaves the slot free, so it isn't + // retried. + if (startAndBind(scope, request) == null && canRetry) { + blockedPublication = scope.blockedPublicationFor(transaction = null) + } + } + + /** + * Retries publication to [scope] after the blocking Activity `ui.load` leaves its slot. Retries + * only within [UI_LOAD_RETRY_WINDOW_SECONDS] of the blocked attempt, and never arms another + * retry. Other blockers do not qualify. + */ + fun retryPublishIfBlocked(scope: IScope) { + val blocked = blockedPublication ?: return + if (blocked.retryDeadline.hasPassed()) { + blockedPublication = null + return + } + val uiLoad = blocked.uiLoad.get() + if (uiLoad != null && !uiLoad.isFinished && scope.transaction === uiLoad) { + // The ui.load transaction still holds the slot; wait for it to leave. + return + } + + publishTo(scope, canRetry = false) } /** Unbinds the transaction from [scope] without finishing it, allowing later publication. */ fun unpublishFrom(scope: IScope) { + blockedPublication = null transaction?.let { unbind(scope, it) } } @@ -102,6 +168,7 @@ internal class StagedNavTransaction(private val scopes: IScopes) { */ fun finish(scope: IScope) { request = null + blockedPublication = null val transaction = transaction ?: return // Forget the transaction before unbinding it: unbinding notifies the host app's scope // observers, which can throw, and a transaction left behind would be bound again later. @@ -116,6 +183,7 @@ internal class StagedNavTransaction(private val scopes: IScopes) { fun forget() { request = null transaction = null + blockedPublication = null } /** Whether the slot is empty, holds a finished transaction, or already holds [transaction]. */ @@ -174,4 +242,30 @@ internal class StagedNavTransaction(private val scopes: IScopes) { ) return false } + + /** + * Returns a blocker only when this scope holds an Activity `ui.load` instead of [transaction]. + * Pass `null` to check whether it blocks a new transaction. + */ + private fun IScope.blockedPublicationFor(transaction: ITransaction?): BlockedPublication? { + val current = this.transaction ?: return null + if (canHold(current, transaction) || current.spanContext.origin != ACTIVITY_UI_LOAD_ORIGIN) { + return null + } + return BlockedPublication( + uiLoad = WeakReference(current), + retryDeadline = Deadline.after(ticker, UI_LOAD_RETRY_WINDOW_SECONDS, TimeUnit.SECONDS), + ) + } + + /** + * A nav publication blocked by [uiLoad], which may be retried until [retryDeadline]. + * + * The `ui.load` transaction is held weakly, as it's only compared with the scope's transaction. + * (A collected transaction can't be bound to the scope anymore.) + */ + private class BlockedPublication( + val uiLoad: WeakReference, + val retryDeadline: Deadline, + ) } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt index 6164f479bab..2b8547b4084 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt @@ -522,6 +522,8 @@ class CoordinatedBackStackObserverTest { private object NoOpNavStatePublisher : INavStatePublisher { override fun publishTo(scope: IScope) = Unit + override fun retryTransactionPublication(scope: IScope) = Unit + override fun unpublishFrom(scope: IScope) = Unit override fun onUnregistered(scope: IScope?) = Unit diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt index 2d6b11eeda5..145b5c0cf74 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt @@ -27,11 +27,19 @@ import androidx.viewpager2.adapter.FragmentStateAdapter import androidx.viewpager2.widget.ViewPager2 import com.google.common.truth.Truth.assertThat import io.sentry.IScope +import io.sentry.IScopes import io.sentry.ISentryClient +import io.sentry.ITransaction import io.sentry.Scope import io.sentry.ScopeType import io.sentry.Scopes -import io.sentry.SentryOptions +import io.sentry.TransactionContext +import io.sentry.TransactionOptions +import io.sentry.android.core.ActivityFramesTracker +import io.sentry.android.core.ActivityLifecycleIntegration +import io.sentry.android.core.BuildInfoProvider +import io.sentry.android.core.SentryAndroidOptions +import io.sentry.android.core.performance.AppStartMetrics import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator import org.junit.After @@ -41,6 +49,7 @@ import org.mockito.kotlin.mock import org.mockito.kotlin.whenever import org.robolectric.Robolectric import org.robolectric.Shadows.shadowOf +import org.robolectric.android.controller.ActivityController import org.robolectric.annotation.Config /** @@ -62,6 +71,7 @@ class SentryNavEffectLifecycleIntegrationTest { @After fun tearDown() { + Fixture.current?.close() Fixture.current = null } @@ -89,6 +99,63 @@ class SentryNavEffectLifecycleIntegrationTest { .inOrder() } + /** + * On API 29+, core's `onActivityPreCreated` callback can run while the previous Activity's Nav3 + * transaction is still bound to the scope. Our Nav3 implementation releases it in its own + * `onActivityPreCreated` callback. Android runs all of those callbacks before + * `onActivityCreated`, when core binds the incoming Activity's `ui.load` transaction, so it takes + * the freed slot. + */ + @Test + fun `an incoming activity binds its own ui load transaction instead of its first nav transaction`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + + val uiLoadTransaction = fixture.uiLoadTransactions.single() + assertThat(uiLoadTransaction.operation).isEqualTo("ui.load") + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + assertThat(homeTransaction.isFinished).isFalse() + assertThat(fixture.scope.screen).isEqualTo("/Checkout") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Checkout") + } + + /** + * Below API 29, there's no `onActivityPreCreated`, so the outgoing Activity's lease is withdrawn + * from `onActivityCreated` if `ActivityLifecycleIntegration` finds that the scope's transaction + * slot is taken. `ActivityLifecycleIntegration` doesn't try again, so its `ui.load` transaction + * remains unbound. The incoming Activity's first nav transaction takes the freed slot instead (a + * known limitation – see `NavLeaseCoordinator`). + */ + @Test + @Config(sdk = [28]) + fun `below API 29, first nav transaction binds when incoming ui load cannot`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + + val uiLoadTransaction = fixture.uiLoadTransactions.single() + assertThat(uiLoadTransaction.isFinished).isFalse() + val checkoutTransaction = requireNotNull(fixture.scope.transaction) + assertThat(checkoutTransaction.name).isEqualTo("/Checkout") + assertThat(checkoutTransaction.operation).isEqualTo("navigation") + assertThat(homeTransaction.isFinished).isFalse() + assertThat(fixture.scope.screen).isEqualTo("/Checkout") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Checkout") + } + @Test fun `an incoming activity's nav data and transaction survive the outgoing activity stopping`() { val fixture = Fixture.create() @@ -215,6 +282,85 @@ class SentryNavEffectLifecycleIntegrationTest { .inOrder() } + /** + * Known limitation: until the closing Activity is destroyed, its unfinished `ui.load` transaction + * keeps the scope's transaction slot, so the Activity returned to has no nav transaction bound + * yet, and spans produced on return attach to the closing Activity's `ui.load` instead. + */ + @Test + fun `going back keeps the covering activity's unfinished ui load bound until it's destroyed`() { + val fixture = Fixture.create() + fixture.returnToHomeWhileCheckoutUiLoadIsUnfinished() + val uiLoadTransaction = fixture.uiLoadTransactions.single() + + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + @Test + fun `going back during the covering activity's ui load starts a nav transaction once it's destroyed`() { + val fixture = Fixture.create() + val (_, checkout) = fixture.returnToHomeWhileCheckoutUiLoadIsUnfinished() + + checkout.stop() + checkout.destroy() + fixture.idle() + + val restarted = requireNotNull(fixture.scope.transaction) + assertThat(restarted.name).isEqualTo("/Home") + assertThat(restarted.operation).isEqualTo("navigation") + assertThat(fixture.uiLoadTransactions.single().isFinished).isTrue() + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + /** + * Below API 29, the covering Activity's `ui.load` transaction only binds if the scope's + * transaction slot is free when the Activity is created (see `NavLeaseCoordinator`), e.g., + * because Home's nav transaction has already finished. + */ + @Test + @Config(sdk = [28]) + fun `below API 29, going back during the covering activity's ui load starts a nav transaction once it's destroyed`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + requireNotNull(fixture.scope.transaction).finish() + home.pause() + val checkout = Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + home.stop() + checkout.pause() + home.restart() + home.resume() + fixture.idle() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.uiLoadTransactions.single()) + + checkout.stop() + checkout.destroy() + fixture.idle() + + val restarted = requireNotNull(fixture.scope.transaction) + assertThat(restarted.name).isEqualTo("/Home") + assertThat(restarted.operation).isEqualTo("navigation") + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + @Test + fun `going back during the covering activity's ui load rebinds the unfinished nav transaction once it's destroyed`() { + val fixture = Fixture.create() + val (_, checkout) = + fixture.returnToHomeWhileCheckoutUiLoadIsUnfinished(finishHomeTransaction = false) + val homeTransaction = fixture.homeTransaction + + checkout.stop() + checkout.destroy() + fixture.idle() + + assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) + assertThat(homeTransaction.isFinished).isFalse() + } + // Activity recreation @Test fun `a recreated activity publishes its own nav data and transaction over the destroyed instance's`() { @@ -830,6 +976,59 @@ class SentryNavEffectLifecycleIntegrationTest { assertThat(homeTransaction.isFinished).isFalse() } + /** + * `ActivityLifecycleIntegration` binds an Activity's `ui.load` transaction when the Activity is + * created, and only if the scope's transaction slot is free. + */ + @Test + fun `an activity without an effect binds its own ui load transaction over a covered activity's nav transaction`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(PlainActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + + val uiLoadTransaction = fixture.uiLoadTransactions.single() + assertThat(uiLoadTransaction.operation).isEqualTo("ui.load") + assertThat(fixture.scope.screen).isEqualTo(PlainActivity::class.java.canonicalName) + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + assertThat(homeTransaction.isFinished).isFalse() + } + + /** + * Below API 29, there's no `onActivityPreCreated`, so the lease is withdrawn from + * `onActivityCreated`, after `ActivityLifecycleIntegration` has found the scope's transaction + * slot taken. `ActivityLifecycleIntegration` doesn't try again, so its `ui.load` transaction + * stays unbound (a known limitation – see `NavLeaseCoordinator`). The covered Activity's nav data + * and transaction are still removed, so they aren't attributed to the Activity. The screen + * `ActivityLifecycleIntegration` set for the Activity is left in place, as it isn't the covered + * Activity's. + */ + @Test + @Config(sdk = [28]) + fun `below API 29, activity without an effect leaves no transaction bound`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(PlainActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + + assertThat(fixture.uiLoadTransactions.single().isFinished).isFalse() + assertThat(fixture.scope.screen).isEqualTo(PlainActivity::class.java.canonicalName) + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + assertThat(homeTransaction.isFinished).isFalse() + } + @Test fun `going back to an activity covered by one without an effect restores its nav data and unfinished transaction`() { val fixture = Fixture.create() @@ -1205,7 +1404,7 @@ class SentryNavEffectLifecycleIntegrationTest { private class Fixture private constructor(tracesSampleRate: Double) { val options = - SentryOptions().apply { + SentryAndroidOptions().apply { dsn = "http://key@localhost/1" defaultScopeType = ScopeType.CURRENT setTracesSampleRate(tracesSampleRate) @@ -1246,6 +1445,74 @@ class SentryNavEffectLifecycleIntegrationTest { events += "$name:$event" } + /** `ui.load` transactions started by [installActivityLifecycleIntegration], in start order. */ + val uiLoadTransactions = mutableListOf() + + /** Home's nav transaction, as set up by [returnToHomeWhileCheckoutUiLoadIsUnfinished]. */ + lateinit var homeTransaction: ITransaction + + /** + * Opens Checkout over Home, binding Checkout's `ui.load` transaction, then returns to Home + * while that `ui.load` transaction is unfinished: Checkout is paused and Home resumed, but + * Checkout isn't stopped or destroyed yet. Finishes Home's nav transaction while Checkout + * covers it unless [finishHomeTransaction] is `false`. + * + * API 29+ only: below API 29, Checkout's `ui.load` transaction can't bind while Home's nav + * transaction holds the scope's transaction slot. + */ + fun returnToHomeWhileCheckoutUiLoadIsUnfinished( + finishHomeTransaction: Boolean = true + ): Pair, ActivityController> { + installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + idle() + homeTransaction = requireNotNull(scope.transaction) + home.pause() + val checkout = Robolectric.buildActivity(CheckoutActivity::class.java).setup() + idle() + home.stop() + idle() + if (finishHomeTransaction) { + homeTransaction.finish() + } + + checkout.pause() + home.restart() + home.resume() + idle() + return home to checkout + } + + private var activityLifecycleIntegration: ActivityLifecycleIntegration? = null + + /** + * Installs a real [ActivityLifecycleIntegration] that traces only instances of + * [tracedActivityClass], so that each such Activity gets a `ui.load` transaction that competes + * with nav transactions for the scope's transaction slot as it does in an app. + * + * Call it before building any Activity: the SDK registers the integration at initialization, so + * its callbacks run before those the coordinator registers when the first effect attaches. + * + * Limiting it to [tracedActivityClass] keeps the other Activities' nav transactions bindable, + * as the tests expect. + */ + fun installActivityLifecycleIntegration(tracedActivityClass: Class) { + val application: Application = ApplicationProvider.getApplicationContext() + val integration = + ActivityLifecycleIntegration( + TracedActivityApplication(application, tracedActivityClass), + BuildInfoProvider(options.logger), + mock(), + ) + integration.register(UiLoadRecordingScopes(scopes, uiLoadTransactions), options) + activityLifecycleIntegration = integration + } + + fun close() { + activityLifecycleIntegration?.close() + AppStartMetrics.getInstance().clear() + } + /** * The host-app setup under test: one `SentryNavEffect` in an Activity's or Fragment's content. */ @@ -1286,6 +1553,123 @@ class SentryNavEffectLifecycleIntegrationTest { } } + /** + * Stands in for [application] when [ActivityLifecycleIntegration] registers its callbacks, and + * relays the application's Activity lifecycle callbacks to it only for instances of + * [tracedActivityClass]. + */ + private class TracedActivityApplication( + private val application: Application, + private val tracedActivityClass: Class, + ) : Application() { + + private val relays = mutableMapOf() + + override fun registerActivityLifecycleCallbacks(callback: ActivityLifecycleCallbacks) { + val relay = TracedActivityCallbacks(callback, tracedActivityClass) + relays[callback] = relay + application.registerActivityLifecycleCallbacks(relay) + } + + override fun unregisterActivityLifecycleCallbacks(callback: ActivityLifecycleCallbacks) { + relays.remove(callback)?.let { application.unregisterActivityLifecycleCallbacks(it) } + } + } + + /** Relays each callback to [delegate] only for instances of [tracedActivityClass]. */ + @Suppress("TooManyFunctions") + private class TracedActivityCallbacks( + private val delegate: Application.ActivityLifecycleCallbacks, + private val tracedActivityClass: Class, + ) : Application.ActivityLifecycleCallbacks { + + private inline fun relay(activity: Activity, block: () -> Unit) { + if (tracedActivityClass.isInstance(activity)) { + block() + } + } + + override fun onActivityPreCreated(activity: Activity, savedInstanceState: Bundle?) = + relay(activity) { delegate.onActivityPreCreated(activity, savedInstanceState) } + + override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = + relay(activity) { delegate.onActivityCreated(activity, savedInstanceState) } + + override fun onActivityPostCreated(activity: Activity, savedInstanceState: Bundle?) = + relay(activity) { delegate.onActivityPostCreated(activity, savedInstanceState) } + + override fun onActivityPreStarted(activity: Activity) = + relay(activity) { delegate.onActivityPreStarted(activity) } + + override fun onActivityStarted(activity: Activity) = + relay(activity) { delegate.onActivityStarted(activity) } + + override fun onActivityPostStarted(activity: Activity) = + relay(activity) { delegate.onActivityPostStarted(activity) } + + override fun onActivityPreResumed(activity: Activity) = + relay(activity) { delegate.onActivityPreResumed(activity) } + + override fun onActivityResumed(activity: Activity) = + relay(activity) { delegate.onActivityResumed(activity) } + + override fun onActivityPostResumed(activity: Activity) = + relay(activity) { delegate.onActivityPostResumed(activity) } + + override fun onActivityPrePaused(activity: Activity) = + relay(activity) { delegate.onActivityPrePaused(activity) } + + override fun onActivityPaused(activity: Activity) = + relay(activity) { delegate.onActivityPaused(activity) } + + override fun onActivityPostPaused(activity: Activity) = + relay(activity) { delegate.onActivityPostPaused(activity) } + + override fun onActivityPreStopped(activity: Activity) = + relay(activity) { delegate.onActivityPreStopped(activity) } + + override fun onActivityStopped(activity: Activity) = + relay(activity) { delegate.onActivityStopped(activity) } + + override fun onActivityPostStopped(activity: Activity) = + relay(activity) { delegate.onActivityPostStopped(activity) } + + override fun onActivityPreSaveInstanceState(activity: Activity, outState: Bundle) = + relay(activity) { delegate.onActivityPreSaveInstanceState(activity, outState) } + + override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = + relay(activity) { delegate.onActivitySaveInstanceState(activity, outState) } + + override fun onActivityPostSaveInstanceState(activity: Activity, outState: Bundle) = + relay(activity) { delegate.onActivityPostSaveInstanceState(activity, outState) } + + override fun onActivityPreDestroyed(activity: Activity) = + relay(activity) { delegate.onActivityPreDestroyed(activity) } + + override fun onActivityDestroyed(activity: Activity) = + relay(activity) { delegate.onActivityDestroyed(activity) } + + override fun onActivityPostDestroyed(activity: Activity) = + relay(activity) { delegate.onActivityPostDestroyed(activity) } + } + + /** Records each `ui.load` transaction started through [delegate] in [uiLoadTransactions]. */ + private class UiLoadRecordingScopes( + private val delegate: IScopes, + private val uiLoadTransactions: MutableList, + ) : IScopes by delegate { + + override fun startTransaction( + transactionContext: TransactionContext, + transactionOptions: TransactionOptions, + ): ITransaction = + delegate.startTransaction(transactionContext, transactionOptions).also { + if (transactionContext.operation == "ui.load") { + uiLoadTransactions += it + } + } + } + private companion object { fun navEffectView(context: Context, route: String): View { diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt index 102d59b35d7..c30e88fd792 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt @@ -743,6 +743,62 @@ class NavLeaseCoordinatorTest { // region lease withdrawal when another Activity covers the leaseholder + @Test + fun `another activity being pre-created while the leaseholder is paused withdraws the nav lease`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityPreCreated(fixture.coveringActivity, null) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `another activity being created while the leaseholder is paused withdraws the lease and unpublishes nav state`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `another activity being created while the leaseholder is resumed keeps the nav lease`() { + val fixture = Fixture.create() + val registration = + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `another activity being created while the leaseholder is paused logs the withdrawal`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + verify(fixture.logger) + .log( + SentryLevel.DEBUG, + "Withdrew the Nav3 lease from a nav root hosted by %s: %s.", + fixture.hostActivity.javaClass.name, + "${fixture.coveringActivity.javaClass.name} was created while the nav root wasn't resumed", + ) + } + @Test fun `another activity resuming while the leaseholder is paused withdraws the nav lease`() { val fixture = Fixture.create() @@ -1024,6 +1080,66 @@ class NavLeaseCoordinatorTest { // endregion + // region retrying nav transaction publication when an Activity is destroyed + + @Test + fun `another activity being post-destroyed retries the leaseholder's transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityPostDestroyed(fixture.coveringActivity) + + assertThat(fixture.retried).containsExactly("owner") + } + + /** Unit tests run against a stubbed `Build.VERSION.SDK_INT` of 0, i.e., below API 29. */ + @Test + fun `below API 29, another activity being destroyed retries the leaseholder's transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityDestroyed(fixture.coveringActivity) + + assertThat(fixture.retried).containsExactly("owner") + } + + @Test + fun `the leaseholder's host activity being destroyed doesn't retry its transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityPostDestroyed(fixture.hostActivity) + + assertThat(fixture.retried).isEmpty() + } + + @Test + fun `an activity being destroyed without a leaseholder retries no transaction publication`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + fixture.activityCallbacks.onActivityPostDestroyed(fixture.coveringActivity) + + assertThat(fixture.retried).isEmpty() + } + + @Test + fun `an activity being destroyed without a scope retries no transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.withoutScope { + fixture.activityCallbacks.onActivityPostDestroyed(fixture.coveringActivity) + } + + assertThat(fixture.retried).isEmpty() + } + + // endregion + // region lease withdrawal when the leaseholder pauses within a resumed Activity @Test @@ -1433,6 +1549,9 @@ class NavLeaseCoordinatorTest { /** Names of the nav states unpublished, in the order they were unpublished. */ val unpublished = mutableListOf() + /** Names of the nav states asked to retry publishing their nav transactions, in order. */ + val retried = mutableListOf() + /** Names of the nav states notified of unregistration, with the scope they were given. */ val unregistered = mutableListOf>() @@ -1444,14 +1563,22 @@ class NavLeaseCoordinatorTest { /** * Publisher named [name] that, by default, records its publication in [published], its - * unpublishing in [unpublished], and its unregistration in [unregistered]. + * transaction publication retries in [retried], its unpublishing in [unpublished], and its + * unregistration in [unregistered]. */ fun publisher( name: String, onPublishTo: (IScope) -> Unit = { published += name }, + onRetryTransactionPublication: (IScope) -> Unit = { retried += name }, onUnpublishFrom: (IScope) -> Unit = { unpublished += name }, onUnregistered: (IScope?) -> Unit = { unregistered += name to it }, - ): INavStatePublisher = FakeNavStatePublisher(onPublishTo, onUnpublishFrom, onUnregistered) + ): INavStatePublisher = + FakeNavStatePublisher( + onPublishTo, + onRetryTransactionPublication, + onUnpublishFrom, + onUnregistered, + ) /** The application of [hostActivity] and [coveringActivity]. */ val application = mock() @@ -1532,11 +1659,14 @@ class NavLeaseCoordinatorTest { private class FakeNavStatePublisher( private val onPublishTo: (IScope) -> Unit, + private val onRetryTransactionPublication: (IScope) -> Unit, private val onUnpublishFrom: (IScope) -> Unit, private val onUnregistered: (IScope?) -> Unit, ) : INavStatePublisher { override fun publishTo(scope: IScope) = onPublishTo(scope) + override fun retryTransactionPublication(scope: IScope) = onRetryTransactionPublication(scope) + override fun unpublishFrom(scope: IScope) = onUnpublishFrom(scope) override fun onUnregistered(scope: IScope?) = onUnregistered.invoke(scope) diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt index 0488c7d1c17..82c45a1df90 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt @@ -13,6 +13,8 @@ import io.sentry.SpanContext import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.TransactionOptions +import io.sentry.time.TestMonotonicTicker +import java.util.concurrent.TimeUnit import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any @@ -31,6 +33,7 @@ class StagedNavTransactionTest { private val finishedTransaction = mock().also { whenever(it.isFinished).thenReturn(true) } private val hostTransaction = transactionWithOrigin("manual") + private val uiLoadTransaction = transactionWithOrigin(ACTIVITY_UI_LOAD_ORIGIN) @Test fun `publishTo() - keeps the started nav transaction when binding it throws`() { @@ -433,6 +436,271 @@ class StagedNavTransactionTest { assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } + @Test + fun `retryPublishIfBlocked() - starts a nav transaction blocked by a ui load once it leaves the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - binds an unfinished nav transaction blocked by a ui load once it leaves the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + assertThat(fixture.startedTransactions).containsExactly(started) + } + + @Test + fun `retryPublishIfBlocked() - starts a nav transaction blocked by a ui load once it finishes`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val uiLoad = transactionWithOrigin(ACTIVITY_UI_LOAD_ORIGIN) + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoad + stagedNavTransaction.publishTo(fixture.scope) + whenever(uiLoad.isFinished).thenReturn(true) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - waits while the blocking ui load holds the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + + fixture.scope.clearTransaction() + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - retries within the retry window`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + fixture.ticker.advance( + TimeUnit.SECONDS.toNanos(UI_LOAD_RETRY_WINDOW_SECONDS) - 1, + TimeUnit.NANOSECONDS, + ) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - measures the retry window from the blocked publication`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS, TimeUnit.SECONDS) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + fixture.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS - 1, TimeUnit.SECONDS) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - retries nothing exactly at the end of the retry window`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + fixture.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS, TimeUnit.SECONDS) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing after the retry window`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + fixture.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS + 1, TimeUnit.SECONDS) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing once the retry window passes while the ui load holds the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.ticker.advance(UI_LOAD_RETRY_WINDOW_SECONDS, TimeUnit.SECONDS) + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `retryPublishIfBlocked() - retries only once`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.transaction = transactionWithOrigin(ACTIVITY_UI_LOAD_ORIGIN) + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing after a transaction other than a ui load blocked it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = hostTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - starts no new nav transaction when the publication wasn't blocked`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.publishTo(fixture.scope) + whenever(started.isFinished).thenReturn(true) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).containsExactly(started) + } + + @Test + fun `retryPublishIfBlocked() - retries nothing after a start the scopes declined`() { + val fixture = Fixture.create(declinesTransactions = true) + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + stagedNavTransaction.publishTo(fixture.scope) + fixture.declinesTransactions = false + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing once the nav transaction is unpublished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing once the nav transaction is finished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + stagedNavTransaction.finish(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `retryPublishIfBlocked() - starts nothing after a ui load blocked a publication that can't be retried`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope, canRetry = false) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - binds nothing after a ui load blocked a rebind that can't be retried`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope, canRetry = false) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isNull() + } + @Test fun `unpublishFrom() - unbinds the started nav transaction without finishing it`() { val fixture = Fixture.create() @@ -613,6 +881,9 @@ class StagedNavTransactionTest { val scope = Scope(SentryOptions()) val logger = mock() + /** Measures [stagedNavTransaction]'s retry window. */ + val ticker = TestMonotonicTicker() + /** Whether [scopes] declines to start transactions, returning a no-op transaction instead. */ var declinesTransactions = false val startedTransactions = mutableListOf() @@ -642,7 +913,7 @@ class StagedNavTransactionTest { } } - val stagedNavTransaction = StagedNavTransaction(scopes) + val stagedNavTransaction = StagedNavTransaction(scopes, ticker) init { scope.options.addScopeObserver(