Feat: Onboarding

.superpowers/sdd/task-20-fixes-report.md

@@ -0,0 +1,130 @@
+# Task 20 whole-branch review fixes
+
+Branch: `feat/onboarding`. All work under `app/feature/feature-onboarding` unless noted.
+
+## Commits
+
+| SHA | Subject |
+| --- | --- |
+| 594f931a5b | Scope the onboarding session cache to the current member |
+| 958d96fad0 | Reset phone step submitting state after save |
+| 5ca6d033ae | Anchor onboarding step content above the bottom buttons |
+| 27e3a45408 | Polish onboarding UI details and comments |
+
+## A. Member-scoped OnboardingSessionStore cache (CRITICAL)
+
+`data/OnboardingSessionStore.kt`:
+- Added internal `fun interface OnboardingMemberIdProvider { fun memberId(): Flow<String?> }` plus
+  `OnboardingMemberIdProviderImpl` (`@ContributesBinding(AppScope::class)`, `@Inject`) delegating to
+  `MemberIdService.getMemberId()`.
+- `OnboardingSession` gained `val memberId: String` as its first property.
+- `OnboardingSessionStore` constructor now takes `memberIdProvider`.
+  - `getOrFetchSession()` (inside the mutex): resolves `currentMemberId` (returns
+    `ErrorMessage("No member id for onboarding session").left()` if null); returns the cached
+    session only when `cachedSession?.memberId == currentMemberId`, otherwise clears the cache and
+    fetches fresh, storing the session stamped with the current member id.
+  - `refreshData()`: resolves the member id, treats a cached session with a different memberId as
+    "no session to refresh" (clears cache, returns the existing error path).
+- `OnboardingGateImpl` (`gate/OnboardingGate.kt`) unchanged and still compiles: it resolves memberId
+  itself then calls `getOrFetchSession()`; the double resolution is harmless.
+- Tests: added `FakeOnboardingMemberIdProvider(var memberId)` to `FakeOnboardingRepository.kt`;
+  updated all 30 `OnboardingSessionStore(...)` construction sites across 10 test files (phone, theme,
+  consent, welcome, coinsured, petid, invite, payment, bundle presenter tests + navigator test) to
+  pass the fake, with the import added (alphabetically before `FakeOnboardingRepository`).
+- New `data/OnboardingSessionStoreTest.kt`:
+  - `same member reuses the cached session without refetching` — one repository response fed, two
+    `getOrFetchSession()` calls return equal sessions, `expectNoEvents()` on the Turbine proves no
+    refetch.
+  - `a member change discards the cache and fetches fresh` — member A response (phone 111) cached,
+    fake switched to member B, second response (phone 222) fed, second call returns the member-B
+    session.
+
+## B. Phone step submit-state bugs
+
+`ui/phone/OnboardingPhoneDestination.kt` submit LaunchedEffect:
+- On success: `currentState = content.copy(isSubmitting = false)` BEFORE `navigator.continueFrom(...)`.
+- On failure: `currentState = (currentState as? OnboardingPhoneUiState.Content ?: content).copy(isSubmitting = false, showSubmissionError = true)` so digits typed during submission survive.
+- Test `save success advances to the next step` updated for the extra emission and asserts the final
+  emitted Content has `isSubmitting == false`.
+
+## C. Single ownership of vertical free space
+
+`ui/OnboardingStepScaffold.kt`: removed the leading `Spacer(Modifier.weight(1f))` from
+`OnboardingStepButtons` (kept its 16dp spacing); added a `Spacer(weight(1f))` to both previews.
+Per-destination arrangement now has exactly one `weight(1f)` split each:
+- welcome: added the second `Spacer(weight(1f))` after the centered content block.
+- phone: added `Spacer(weight(1f))` between the text field and the buttons.
+- invite: added `Spacer(weight(1f))` before and after the centered code text.
+- bundle: added `Spacer(weight(1f))` after the rows loop.
+- consent, theme, coinsured, petid, payment (both connected/not-connected): already had the single
+  weight in the required place, left unchanged.
+
+## D. Small quality fixes
+
+1. `OnboardingStepHeader`: `Spacer(Modifier.height(4.dp))` between title and description.
+2. `ui/theme/OnboardingThemeDestination.kt`: (a) load-effect initial Content now uses
+   `selectedTheme = Theme.SYSTEM_DEFAULT` with comment `// Placeholder; the return site merges the
+   live stored theme.` (b) `ThemeOptionRow` internal padding changed `padding(16.dp)` ->
+   `padding(vertical = 16.dp)`; call sites keep horizontal 16dp so the total horizontal inset is 16dp
+   and the ripple (clip+clickable before the padding) still covers the row.
+3. `navigation/OnboardingNavigator.kt`: removed the dead `navigateBack()` (no source references).
+4. `tracking-firebase/.../ConsentAwareEventTrackingClient.kt`: added the public-hints comment on the
+   providers interface (ktlint moved it above the annotations, which is the correct spot).
+5. `build.gradle.kts`: comment above `isReturnDefaultValues = true`.
+6. Removed redundant `val content = state` aliases in invite and bundle destinations (use the
+   smart-cast `state` directly).
+
+## Verification
+
+Commands run (grepped for BUILD SUCCESSFUL, piped exit codes not trusted):
+
+- `./gradlew :feature-onboarding:testDebugUnitTest :tracking-firebase:testDebugUnitTest` -> BUILD SUCCESSFUL
+- `./gradlew :app:compileDebugKotlin` -> BUILD SUCCESSFUL
+- `./gradlew ktlintFormat` -> BUILD SUCCESSFUL (only reflowed the new tracking-firebase comment,
+  committed in D)
+
+Working tree clean after the four commits.
+
+## Deviations
+
+None. The theme ThemeOptionRow modifier order (clip -> clickable -> padding) already produced a
+ripple covering the row, so no reorder beyond the padding change was needed.
+
+## Fix: DENIED disables SDK collection
+
+### What changed
+
+`ConsentAwareEventTrackingClient.kt`:
+- Added `private var collectionRequestedEnabled: Boolean = true` (guarded by `lock`).
+- `setCollectionEnabled(enabled)` now stores the value under the lock, then calls
+  `applyCollectionEnabled()` instead of delegating directly.
+- New `private fun applyCollectionEnabled()`: computes `effective = collectionRequestedEnabled &&
+  consent != DENIED` under the lock, then calls `delegate.setCollectionEnabled(effective)` outside
+  it. NOT_DECIDED and GRANTED leave collection enabled (subject to the demo gate); only DENIED forces
+  it off; flipping back to GRANTED re-enables if the demo gate allows.
+- The consent collector's `init` block now calls `applyCollectionEnabled()` after every consent
+  update.
+- KDoc updated to document the DENIED-disables-SDK-collection behavior.
+
+`ConsentAwareEventTrackingClientTest.kt`:
+- `RecordingClient.setCollectionEnabled` now records each call into `collectionEnabledCalls`.
+- Three new tests added:
+  - `denying consent disables SDK collection and granting re-enables it`
+  - `demo mode wins over granted consent`
+  - `enabling collection while denied stays disabled`
+
+`docs/superpowers/specs/2026-07-21-onboarding-design.md`:
+- Appended one sentence to the "Analytics consent mechanism" section documenting the post-review
+  product decision.
+
+### Commands run
+
+```
+./gradlew :tracking-firebase:testDebugUnitTest 2>&1 | tee /tmp/test_output.txt
+grep "BUILD SUCCESSFUL" /tmp/test_output.txt  -> BUILD SUCCESSFUL in 1m 3s
+
+./gradlew ktlintFormat 2>&1 | tee /tmp/ktlint_output.txt
+grep "BUILD SUCCESSFUL" /tmp/ktlint_output.txt  -> BUILD SUCCESSFUL in 41s
+```
+
+Both BUILD SUCCESSFUL outputs confirmed. ktlintFormat made no further changes to the modified files.

CLAUDE.md

@@ -482,14 +482,16 @@ Configuration in `.editorconfig`:
 
 ### Comments
 
-Code comments and KDoc must describe the **current** code and stand on their own. Before writing one, apply the test: *would this make sense to someone reading the file cold, with no knowledge of the PR, the conversation, or what was decided against?* If not, it does not belong in the source. Do not reference:
+Code comments and KDoc must describe the **current** code and stand on their own. A comment fails to earn its place in two ways: it tells the wrong kind of story, or it repeats what is already there. Before writing one, apply the test: *would this make sense to someone reading the file cold, with no knowledge of the PR, the conversation, or what was decided against?* If not, it does not belong in the source. Do not reference:
 
 - **History / migration:** "Replaces Nav2…", "used to live in…".
 - **Rejected alternatives:** "…not the iOS glass", "instead of the old Y".
 - **Conversation / design-process state:** "pending design", "for now", "TBD".
 
 When tempted to write "X instead of Y", drop the Y half and justify X on its own terms. If that leaves nothing, the code was self-explanatory, so delete the comment. Migration/history/process context belongs in the commit message.
 
+**Say only what the code and its neighbours don't.** A comment must add information not already visible at its location. Do not restate a signature, a type, or a contract that a nearby or interface KDoc already documents; a paraphrase of something visible at the same spot is noise. Keep it to the non-obvious point (usually the *why*); if it grows into several lines re-describing mechanics that the code already shows, cut it back. If the same point already lives elsewhere (an interface's KDoc, a constant's own comment), state it once, at the spot that owns it, rather than repeating it at each use. Prefer no comment over a redundant or verbose one.
+
 ## Working with GraphQL
 
 ### Apollo Schema

app/app/build.gradle.kts

@@ -207,6 +207,7 @@ dependencies {
   implementation(projects.featureLogin)
   implementation(projects.featureMovingflow)
   implementation(projects.featureMovingflowNavigation)
+  implementation(projects.featureOnboarding)
 
   implementation(projects.featureRemoveAddons)
   implementation(projects.featurePayoutAccount)

app/app/src/main/kotlin/com/hedvig/android/app/MainActivity.kt

@@ -42,6 +42,7 @@ import com.hedvig.android.core.demomode.DemoManager
 import com.hedvig.android.core.rive.RiveInitializer
 import com.hedvig.android.core.tracking.EventTrackingClient
 import com.hedvig.android.data.settings.datastore.SettingsDataStore
+import com.hedvig.android.feature.onboarding.data.ResetOnboardingSeenUseCase
 import com.hedvig.android.featureflags.FeatureManager
 import com.hedvig.android.language.LanguageLaunchCheckUseCase
 import com.hedvig.android.language.LanguageService
@@ -114,6 +115,9 @@ class MainActivity : AppCompatActivity() {
   @Inject
   private lateinit var serializersModules: Set<SerializersModule>
 
+  @Inject
+  private lateinit var resetOnboardingSeenUseCase: ResetOnboardingSeenUseCase
+
   /**
    * Per-Activity host for the navigation state. A retained `ViewModel`, so it (and the
    * [BackstackController] it owns) survives a config change but dies with this Activity — giving each
@@ -274,6 +278,8 @@ class MainActivity : AppCompatActivity() {
           currentDestinationHolder = currentDestinationHolder,
           eventTrackingClient = eventTrackingClient,
           screenParameterExtractor = screenParameterExtractor,
+          onboardingGate = navRetainedViewModel.onboardingGate,
+          resetOnboardingSeenUseCase = resetOnboardingSeenUseCase,
         )
       }
     }

app/app/src/main/kotlin/com/hedvig/android/app/di/ActivityRetainedGraph.kt

@@ -4,6 +4,7 @@ import com.hedvig.android.app.navigation.BackstackController
 import com.hedvig.android.app.navigation.SessionReconciler
 import com.hedvig.android.core.common.di.ActivityRetainedScope
 import com.hedvig.android.core.common.di.AppScope
+import com.hedvig.android.feature.onboarding.gate.OnboardingGate
 import com.hedvig.android.navigation.compose.Backstack
 import dev.zacsweers.metro.ContributesTo
 import dev.zacsweers.metro.GraphExtension
@@ -28,6 +29,8 @@ import dev.zacsweers.metrox.viewmodel.MetroViewModelMultibindings
 internal interface ActivityRetainedGraph : MetroViewModelMultibindings {
   val sessionReconciler: SessionReconciler
 
+  val onboardingGate: OnboardingGate
+
   @Provides
   fun bindBackstack(controller: BackstackController): Backstack = controller
 

app/app/src/main/kotlin/com/hedvig/android/app/navigation/HedvigEntryProvider.kt

@@ -48,6 +48,8 @@ import com.hedvig.android.feature.login.navigation.loginEntries
 import com.hedvig.android.feature.movingflow.MovingSource
 import com.hedvig.android.feature.movingflow.SelectContractForMovingKey
 import com.hedvig.android.feature.movingflow.movingFlowEntries
+import com.hedvig.android.feature.onboarding.data.ResetOnboardingSeenUseCase
+import com.hedvig.android.feature.onboarding.navigation.onboardingEntries
 import com.hedvig.android.feature.payments.navigation.paymentsEntries
 import com.hedvig.android.feature.payoutaccount.navigation.PayoutAccountKey
 import com.hedvig.android.feature.payoutaccount.navigation.payoutAccountEntries
@@ -57,6 +59,7 @@ import com.hedvig.android.feature.terminateinsurance.navigation.TerminateInsuran
 import com.hedvig.android.feature.terminateinsurance.navigation.terminateInsuranceEntries
 import com.hedvig.android.feature.travelcertificate.navigation.TravelCertificateKey
 import com.hedvig.android.feature.travelcertificate.navigation.travelCertificateEntries
+import com.hedvig.android.language.Language
 import com.hedvig.android.language.LanguageService
 import com.hedvig.android.logger.logcat
 import com.hedvig.android.memberquickactions.InnerHelpCenterDestination
@@ -97,6 +100,7 @@ internal fun EntryProviderScope<HedvigNavKey>.hedvigEntryProvider(
   imageLoader: ImageLoader,
   languageService: LanguageService,
   hedvigBuildConstants: HedvigBuildConstants,
+  resetOnboardingSeenUseCase: ResetOnboardingSeenUseCase,
 ) {
   val shouldShowRequestPermissionRationale: (String) -> Boolean = androidAppHost::shouldShowPermissionRationale
   val navigateToConnectPayment: () -> Unit = { backstack.add(TrustlyKey) }
@@ -167,6 +171,13 @@ internal fun EntryProviderScope<HedvigNavKey>.hedvigEntryProvider(
     navigateToConnectPayment = navigateToConnectPayment,
     navigateToPayoutAccount = navigateToPayoutAccount,
     navigateToNewConversation = navigateToNewConversation,
+    onResetOnboardingForDebug = {
+      scope.launch {
+        resetOnboardingSeenUseCase.invoke()
+        // Debug-only feedback for non-production builds; deliberately not translated.
+        globalSnackBarState.show("Onboarding reset. Background and foreground the app to see it.")
+      }
+    },
   )
   addChatEntries(
     backstack = backstack,
@@ -184,9 +195,19 @@ internal fun EntryProviderScope<HedvigNavKey>.hedvigEntryProvider(
     externalNavigator = externalNavigator,
     navigateToNewConversation = navigateToNewConversation,
     navigateToInbox = navigateToInbox,
+    openPrivacyPolicy = { openUrl(privacyPolicyUrl(languageService.getLanguage())) },
   )
 }
 
+/**
+ * The public privacy-policy page, per language. Mirrors the links used by the profile "about app"
+ * screen; kept here so `:app` owns the URL rather than `feature-onboarding`.
+ */
+private fun privacyPolicyUrl(language: Language): String = when (language) {
+  Language.SV_SE -> "https://www.hedvig.com/se/hedvig/personuppgifter"
+  Language.EN_SE -> "https://www.hedvig.com/se-en/hedvig/privacy-policy"
+}
+
 private fun EntryProviderScope<HedvigNavKey>.addLoginEntries(
   backstack: BackstackController,
   hedvigBuildConstants: HedvigBuildConstants,
@@ -463,6 +484,7 @@ private fun EntryProviderScope<HedvigNavKey>.addProfileEntries(
   navigateToConnectPayment: () -> Unit,
   navigateToPayoutAccount: () -> Unit,
   navigateToNewConversation: () -> Unit,
+  onResetOnboardingForDebug: () -> Unit,
 ) {
   profileEntries(
     settingsDestinationNestedGraphs = {
@@ -489,8 +511,10 @@ private fun EntryProviderScope<HedvigNavKey>.addProfileEntries(
     onNavigateToTravelCertificate = { backstack.add(TravelCertificateKey) },
     onNavigateToInsuranceEvidence = { backstack.add(InsuranceEvidenceKey) },
     openUrl = openUrl,
+    openPrivacyPolicy = { openUrl(privacyPolicyUrl(languageService.getLanguage())) },
     navigateToChipId = { backstack.add(ChipIdKey()) },
     languageService = languageService,
+    onResetOnboardingForDebug = onResetOnboardingForDebug,
   )
 }
 
@@ -531,6 +555,7 @@ private fun EntryProviderScope<HedvigNavKey>.addSharedFlowEntries(
   externalNavigator: ExternalNavigator,
   navigateToNewConversation: () -> Unit,
   navigateToInbox: () -> Unit,
+  openPrivacyPolicy: () -> Unit,
 ) {
   addonPurchaseEntries(
     backstack = backstack,
@@ -566,4 +591,11 @@ private fun EntryProviderScope<HedvigNavKey>.addSharedFlowEntries(
   )
   imageViewerEntries(backstack, imageLoader)
   removeAddonsEntries(backstack = backstack)
+  onboardingEntries(
+    backstack = backstack,
+    imageLoader = imageLoader,
+    openUrl = openUrl,
+    openPrivacyPolicy = openPrivacyPolicy,
+    navigateToChipId = { contractId -> backstack.add(ChipIdKey(contractId)) },
+  )
 }

app/app/src/main/kotlin/com/hedvig/android/app/navigation/NavRetainedViewModel.kt

@@ -7,6 +7,7 @@ import androidx.lifecycle.ViewModel
 import com.hedvig.android.app.di.ActivityRetainedGraph
 import com.hedvig.android.app.di.AppGraph
 import com.hedvig.android.app.di.MergedMetroViewModelFactory
+import com.hedvig.android.feature.onboarding.gate.OnboardingGate
 import dev.zacsweers.metro.asContribution
 import dev.zacsweers.metrox.viewmodel.MetroViewModelFactory
 
@@ -39,5 +40,7 @@ internal class NavRetainedViewModel(appGraph: AppGraph, isOwnTask: Boolean) : Vi
 
   val sessionReconciler: SessionReconciler = activityGraph.sessionReconciler
 
+  val onboardingGate: OnboardingGate = activityGraph.onboardingGate
+
   val viewModelFactory: MetroViewModelFactory = MergedMetroViewModelFactory(appGraph, activityGraph)
 }