diff --git a/app/README.md b/app/README.md index e2da3c40a..6b354c1bd 100644 --- a/app/README.md +++ b/app/README.md @@ -16,6 +16,7 @@ ## Composition root and startup - `AppContainer` is created once in `BoxLoreApplication.onCreate` and wires database, network, auth, RSS, catalog, ranking, playback, queue, downloads, prefs, support and analytics in the [object graph order](../ARCHITECTURE.md#identity-and-storage). +- `ui/rememberOnboardingViewModel` keeps the one `OnboardingViewModel` in the activity's `ViewModelStore`, so every onboarding flow shares it and it is cleared when the activity finishes; never create it with `remember`. - The one `PlaybackRepository` is built lazily. Cast state waits in `DeferredCastSessionState` until it exists, so widget, worker and notification starts never build playback on the main thread. - The container gives core modules ports instead of reverse dependencies: `feedTelemetry` for feed events, `rolloutSwitches`, and a `DownloadServiceLauncherHolder` that starts `MediaDownloadService`. - `onCreate` order: API sessions first, then the Coil image loader (before widgets and services), Media3 Cast from `BoxLoreCastOptionsProvider`, the shared `UserPreferencesRepository`, the container and its holders. PostHog starts later on a background thread through `PostHogStartup`, after `AnalyticsHelper.initPolicy`. diff --git a/app/src/main/java/cx/aswin/boxlore/ui/BoxLoreAppRoot.kt b/app/src/main/java/cx/aswin/boxlore/ui/BoxLoreAppRoot.kt index ade05ebf2..910cc9be6 100644 --- a/app/src/main/java/cx/aswin/boxlore/ui/BoxLoreAppRoot.kt +++ b/app/src/main/java/cx/aswin/boxlore/ui/BoxLoreAppRoot.kt @@ -240,8 +240,9 @@ fun BoxLoreAppRoot( FcmTopicHelper.reconcileAfterRestoreIfNeeded(activity, subscriptionRepository) } + // One instance for every onboarding flow, held by the activity so it is cleared when the activity finishes. val onboardingViewModel = - remember { + rememberOnboardingViewModel(owner = activity) { cx.aswin.boxlore.feature.onboarding.OnboardingViewModel( application, podcastRepository, diff --git a/app/src/main/java/cx/aswin/boxlore/ui/OnboardingViewModelScope.kt b/app/src/main/java/cx/aswin/boxlore/ui/OnboardingViewModelScope.kt new file mode 100644 index 000000000..b56671322 --- /dev/null +++ b/app/src/main/java/cx/aswin/boxlore/ui/OnboardingViewModelScope.kt @@ -0,0 +1,38 @@ +package cx.aswin.boxlore.ui + +import androidx.compose.runtime.Composable +import androidx.lifecycle.ViewModel +import androidx.lifecycle.ViewModelProvider +import androidx.lifecycle.ViewModelStoreOwner +import androidx.lifecycle.viewmodel.compose.viewModel +import cx.aswin.boxlore.feature.onboarding.OnboardingViewModel + +/** The key the activity's onboarding view model is stored under. */ +internal const val ONBOARDING_VIEW_MODEL_KEY = "boxlore-onboarding" + +/** + * The activity's one [OnboardingViewModel], kept in [owner]'s view model store rather than in + * composition. + * + * Every onboarding path shares this instance: search, Echo (AI), genre picks, the import sheet, + * Home's AI onboarding entry and the sign-in detour through Settings. Shows picked in one flow + * therefore carry over to the others, and the instance survives the activity being recreated. + * + * Because the store owns it, `onCleared` runs when the activity finishes for good: Echo requests + * are cancelled, deck audio is released, and an onboarding left unfinished is reported as + * abandoned. Created with `remember`, it was never cleared, so none of that happened. + * + * [build] runs once per store; later calls return the stored instance. + */ +@Composable +internal fun rememberOnboardingViewModel( + owner: ViewModelStoreOwner, + build: () -> OnboardingViewModel, +): OnboardingViewModel = viewModel( + viewModelStoreOwner = owner, + key = ONBOARDING_VIEW_MODEL_KEY, + factory = object : ViewModelProvider.Factory { + @Suppress("UNCHECKED_CAST") + override fun create(modelClass: Class): T = build() as T + }, +) diff --git a/app/src/test/java/cx/aswin/boxlore/ui/OnboardingViewModelRecreationTest.kt b/app/src/test/java/cx/aswin/boxlore/ui/OnboardingViewModelRecreationTest.kt new file mode 100644 index 000000000..6308258f6 --- /dev/null +++ b/app/src/test/java/cx/aswin/boxlore/ui/OnboardingViewModelRecreationTest.kt @@ -0,0 +1,111 @@ +package cx.aswin.boxlore.ui + +import android.app.Application +import android.os.Looper +import androidx.activity.ComponentActivity +import androidx.activity.compose.setContent +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import cx.aswin.boxlore.core.analytics.AnalyticsHelper +import cx.aswin.boxlore.core.catalog.PodcastRepository +import cx.aswin.boxlore.core.catalog.SubscriptionRepository +import cx.aswin.boxlore.core.database.BoxLoreDatabase +import cx.aswin.boxlore.core.prefs.BoxcastPrefs +import cx.aswin.boxlore.core.prefs.UserPreferencesRepository +import cx.aswin.boxlore.core.rss.RssPodcastRepository +import cx.aswin.boxlore.core.testing.TestFixtures +import cx.aswin.boxlore.feature.onboarding.OnboardingViewModel +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import org.robolectric.android.controller.ActivityController +import org.robolectric.annotation.Config + +/** + * A real activity recreated mid-onboarding keeps its onboarding view model and the shows picked so + * far, and reports nothing; only finishing it for good clears the view model and reports the + * unfinished onboarding as abandoned. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34], application = Application::class) +class OnboardingViewModelRecreationTest { + private val context: Application = ApplicationProvider.getApplicationContext() + private lateinit var database: BoxLoreDatabase + private lateinit var controller: ActivityController + private val events = mutableListOf>>() + private lateinit var restoreAnalytics: () -> Unit + private var builds = 0 + + @Before + fun setUp() { + database = Room.inMemoryDatabaseBuilder(context, BoxLoreDatabase::class.java).allowMainThreadQueries().build() + RssPodcastRepository.clearInstanceForTests() + BoxcastPrefs(context).setOnboardingCompleted(false) + restoreAnalytics = AnalyticsHelper.installRecordingSink(events) + controller = Robolectric.buildActivity(ComponentActivity::class.java).setup() + } + + @After + fun tearDown() { + restoreAnalytics() + database.close() + RssPodcastRepository.clearInstanceForTests() + } + + private fun build(): OnboardingViewModel { + builds++ + val podcastRepository = + PodcastRepository( + baseUrl = "http://localhost/", + publicKey = "test-app-key", + context = context, + rssRepository = RssPodcastRepository.createForTests(context = context, database = database), + ) + return OnboardingViewModel(context, podcastRepository, SubscriptionRepository(database.podcastDao()), UserPreferencesRepository(context)) + } + + /** Sets the activity's content the way the app root does and returns the view model it gets. */ + private fun showOnboarding(): OnboardingViewModel { + var viewModel: OnboardingViewModel? = null + val activity = controller.get() + activity.setContent { viewModel = rememberOnboardingViewModel(owner = activity, build = ::build) } + shadowOf(Looper.getMainLooper()).idle() + assertNotNull(viewModel) + return viewModel!! + } + + @Test + fun `recreating the activity mid-onboarding keeps the view model and its picks and reports nothing`() { + val before = showOnboarding() + before.onWelcomeScreenViewed() + before.setShowsSelected(listOf(TestFixtures.podcast(id = "show-1"), TestFixtures.podcast(id = "show-2")), selected = true) + + controller.recreate() + val after = showOnboarding() + + assertSame(before, after) + assertEquals(1, builds) + assertEquals(setOf("show-1", "show-2"), after.uiState.value.selectedPodcasts.keys) + assertTrue(events.none { it.first == "onboarding_abandoned" }) + } + + @Test + fun `finishing the activity after a recreation reports the unfinished onboarding once`() { + showOnboarding().onWelcomeScreenViewed() + controller.recreate() + showOnboarding() + + controller.get().finish() + controller.pause().stop().destroy() + + assertEquals(1, events.count { it.first == "onboarding_abandoned" }) + } +} diff --git a/app/src/test/java/cx/aswin/boxlore/ui/OnboardingViewModelScopeTest.kt b/app/src/test/java/cx/aswin/boxlore/ui/OnboardingViewModelScopeTest.kt new file mode 100644 index 000000000..2aefb6445 --- /dev/null +++ b/app/src/test/java/cx/aswin/boxlore/ui/OnboardingViewModelScopeTest.kt @@ -0,0 +1,142 @@ +package cx.aswin.boxlore.ui + +import android.app.Application +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.lifecycle.ViewModelStore +import androidx.lifecycle.ViewModelStoreOwner +import androidx.lifecycle.viewModelScope +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import cx.aswin.boxlore.core.analytics.AnalyticsHelper +import cx.aswin.boxlore.core.catalog.PodcastRepository +import cx.aswin.boxlore.core.catalog.SubscriptionRepository +import cx.aswin.boxlore.core.database.BoxLoreDatabase +import cx.aswin.boxlore.core.prefs.BoxcastPrefs +import cx.aswin.boxlore.core.prefs.UserPreferencesRepository +import cx.aswin.boxlore.core.rss.RssPodcastRepository +import cx.aswin.boxlore.feature.onboarding.OnboardingViewModel +import cx.aswin.boxlore.feature.onboarding.markOnboardingCompletedSilent +import kotlinx.coroutines.isActive +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * The onboarding view model lives in the activity's view model store: every onboarding flow shares + * one instance (so picks carry over between search, Echo and the import sheet), and finishing the + * activity clears it. Created with `remember` it was never cleared, so an unfinished onboarding was + * never reported as abandoned and its Echo work was never cancelled. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34], application = Application::class) +class OnboardingViewModelScopeTest { + @get:Rule val composeRule = createComposeRule() + + private val context: Application = ApplicationProvider.getApplicationContext() + private lateinit var database: BoxLoreDatabase + private val store = ViewModelStore() + private val activityOwner = object : ViewModelStoreOwner { + override val viewModelStore: ViewModelStore = store + } + private val events = mutableListOf>>() + private lateinit var restoreAnalytics: () -> Unit + private var builds = 0 + + @Before + fun setUp() { + database = Room.inMemoryDatabaseBuilder(context, BoxLoreDatabase::class.java).allowMainThreadQueries().build() + RssPodcastRepository.clearInstanceForTests() + BoxcastPrefs(context).setOnboardingCompleted(false) + restoreAnalytics = AnalyticsHelper.installRecordingSink(events) + } + + @After + fun tearDown() { + store.clear() + restoreAnalytics() + database.close() + RssPodcastRepository.clearInstanceForTests() + } + + private fun build(): OnboardingViewModel { + builds++ + val podcastRepository = + PodcastRepository( + baseUrl = "http://localhost/", + publicKey = "test-app-key", + context = context, + rssRepository = RssPodcastRepository.createForTests(context = context, database = database), + ) + return OnboardingViewModel(context, podcastRepository, SubscriptionRepository(database.podcastDao()), UserPreferencesRepository(context)) + } + + @Test + fun `every onboarding flow gets the same instance while the activity lives`() { + var showImportSheet by mutableStateOf(false) + var showOnboarding by mutableStateOf(true) + val seen = mutableListOf() + composeRule.setContent { + // Separate call sites stand for the onboarding route, the import sheet and Home's AI entry. + if (showOnboarding) seen += rememberOnboardingViewModel(activityOwner, ::build) + if (showImportSheet) seen += rememberOnboardingViewModel(activityOwner, ::build) + } + + composeRule.runOnIdle { showImportSheet = true } + // Leaving onboarding (e.g. for the sign-in detour) and coming back keeps the same instance. + composeRule.runOnIdle { showOnboarding = false } + composeRule.runOnIdle { showOnboarding = true } + composeRule.waitForIdle() + + assertEquals(1, builds) + assertTrue(seen.size >= 3) + seen.forEach { assertSame(seen.first(), it) } + } + + @Test + fun `finishing the activity mid-onboarding reports it abandoned once and stops its work`() { + lateinit var viewModel: OnboardingViewModel + composeRule.setContent { viewModel = rememberOnboardingViewModel(activityOwner, ::build) } + composeRule.runOnIdle { viewModel.onWelcomeScreenViewed() } + + composeRule.runOnIdle { store.clear() } + + val abandoned = events.filter { it.first == "onboarding_abandoned" } + assertEquals(1, abandoned.size) + assertTrue(abandoned.single().second.containsKey("last_step")) + assertFalse(viewModel.viewModelScope.isActive) + } + + @Test + fun `a finished onboarding is not reported when the activity finishes`() { + lateinit var viewModel: OnboardingViewModel + composeRule.setContent { viewModel = rememberOnboardingViewModel(activityOwner, ::build) } + composeRule.runOnIdle { + viewModel.onWelcomeScreenViewed() + viewModel.markOnboardingCompletedSilent {} + } + + composeRule.runOnIdle { store.clear() } + + assertTrue(events.none { it.first == "onboarding_abandoned" }) + } + + @Test + fun `an onboarding never started is not reported when the activity finishes`() { + composeRule.setContent { rememberOnboardingViewModel(activityOwner, ::build) } + + composeRule.runOnIdle { store.clear() } + + assertTrue(events.none { it.first == "onboarding_abandoned" }) + } +} diff --git a/feature/onboarding/README.md b/feature/onboarding/README.md index e54e5e812..a806d2e9e 100644 --- a/feature/onboarding/README.md +++ b/feature/onboarding/README.md @@ -72,7 +72,7 @@ src/main/res/values/ # echo_onboarding.xml, onboarding_suggest ## Threading / lifecycle -- `OnboardingViewModel` is constructed directly in the app root's composition (`remember`), not through a `ViewModelStore`, so it outlives the onboarding route. Its `onCleared` cancels Echo and deck requests, releases the clip pool and records abandonment. +- `OnboardingViewModel` is one instance per activity, kept in the activity's `ViewModelStore` by `:app` (`rememberOnboardingViewModel`). Every onboarding flow shares it (search, Echo, genres, the import sheet, Home's AI entry), so picks carry over between them. Its `onCleared` runs when the activity finishes: it cancels Echo and deck requests, releases the clip pool and reports an unfinished onboarding as abandoned. - Echo turn, synthesis, deck and similar-shows jobs cancel on Back, Search, the topic fallback, restart and skip. Cancelled work never replaces a restored question or shows an error; cancellation is rethrown, not reported. - Genre synthesis runs `async` on `Dispatchers.IO` and catches failure inside the child (`orFallbackOnFailure`), because an exception escaping an `async` child fails its parent. - Subscribing a show asks the application-scoped `subscriptionForegroundSync` for a catalog ingest, so leaving onboarding cannot cancel it.