Skip to content

SDKS-4915 Create UI to test progress-tracked migration in the Test app#180

Open
rodrigoareis wants to merge 3 commits intodevelopfrom
SDKS-4915
Open

SDKS-4915 Create UI to test progress-tracked migration in the Test app#180
rodrigoareis wants to merge 3 commits intodevelopfrom
SDKS-4915

Conversation

@rodrigoareis
Copy link
Copy Markdown
Contributor

@rodrigoareis rodrigoareis commented Apr 8, 2026

JIRA Ticket

SDKS-4915 Create Example UI demonstrating progress-tracked migration in the Test app

Description

Summary

  • Adds an Auth Migration testing screen to pingsampleapp that allows developers to test migration of legacy ForgeRock Authenticator credentials (OATH and Push) to the modern Ping SDK storage format
  • Mirrors the existing iOS implementation (AuthMigrationView / AuthMigrationViewModel) adapted for Android with Jetpack Compose and Material 3
  • Wires the new screen into the sample app's navigation under the MFA section on the home screen

Changes

New Files

  • AuthMigrationScreen.kt — Composable UI with 4 card sections: Instructions (numbered steps), Migration Status (with dynamic badge), Progress (step list with status icons), and Result (success/error)
  • AuthMigrationViewModel.kt — ViewModel that checks for legacy data via DefaultStorageClientProvider.isMigrationRequired() and runs AuthMigration.start() with real-time progress tracking through FlowCollector<MigrationProgress>

Modified Files

  • build.gradle.kts — Added mfa:auth-migration and foundation:migration dependencies; added pickFirsts rule for libtool-file.so JNI conflict between device-root and transitive forgerock-core
  • Navigation.kt — Added AUTH_MIGRATION route and composable entry point
  • HomeApp.kt — Added onAuthMigrationClick callback and IconRowItem card (SwapHoriz icon) in the MFA section
  • strings.xml — Added 22 string resources for the migration screen

Summary by CodeRabbit

  • New Features

    • Added an authentication migration flow with a dedicated screen to detect legacy FR Authenticator credentials, run migration with real-time per-step progress/status, and show clear success or error results. New UI strings for titles, instructions, status labels, and progress messages included.
  • Chores

    • Build configuration updated to handle duplicate native libraries during packaging and include migration-related modules.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

📝 Walkthrough

Walkthrough

Adds an authentication migration feature: new Gradle deps and packaging rule, a migration ViewModel and Compose screen, navigation entry and Home integration, plus localized strings for a legacy-to-SDK credential migration flow.

Changes

Cohort / File(s) Summary
Gradle Configuration
samples/pingsampleapp/build.gradle.kts
Added implementation(project(":mfa:auth-migration")), implementation(project(":foundation:migration")), and packaging { jniLibs { pickFirsts += "lib/*/libtool-file.so" } }.
Auth Migration (ViewModel & UI)
samples/pingsampleapp/src/main/java/.../authmigration/AuthMigrationViewModel.kt, samples/pingsampleapp/src/main/java/.../authmigration/AuthMigrationScreen.kt
New AuthMigrationViewModel (enums, state, checkMigrationNeeded, startMigration) and AuthMigrationScreen composable with instructions, status badge, start button, per-step progress, and result cards.
Navigation & Home Integration
samples/pingsampleapp/src/main/java/.../navigation/Navigation.kt, samples/pingsampleapp/src/main/java/.../home/HomeApp.kt
Added Route.AUTH_MIGRATION, registered a composable destination that creates the ViewModel and shows the screen; HomeApp gains onAuthMigrationClick param and an MFA row to navigate to the migration screen.
String Resources
samples/pingsampleapp/src/main/res/values/strings.xml
Added strings for titles, instructions, status labels, start button, progress title, and empty-state messaging for auth migration UI.

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant Screen as AuthMigrationScreen
    participant VM as AuthMigrationViewModel
    participant Storage as DefaultStorageClientProvider
    participant API as AuthMigration

    User->>Screen: Open migration screen
    Screen->>VM: checkMigrationNeeded(context)
    VM->>VM: set migrationStatus = CHECKING
    VM->>Storage: isMigrationRequired(...)
    Storage-->>VM: returns Boolean
    VM->>VM: set migrationStatus = IDLE, isMigrationNeeded

    User->>Screen: Tap "Start Migration"
    Screen->>VM: startMigration(context)
    VM->>VM: set migrationStatus = RUNNING, clear results
    VM->>API: AuthMigration.start(context, logger, progressCollector)

    loop per migration step
      API->>VM: Progress.StepStarted(step)
      VM->>VM: add StepResult(IN_PROGRESS)
      VM-->>Screen: state update
      API->>VM: Progress.StepCompleted(step)
      VM->>VM: update StepResult -> COMPLETED
      VM-->>Screen: state update
    end

    API-->>VM: Migration.Success(summary)
    VM->>VM: set migrationStatus = COMPLETED, summaryMessage
    VM-->>Screen: final state (COMPLETED)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through code with nimble paws,
Found old keys tucked in legacy jaws.
Steps lit up as I nudged them on,
From buried burrows the tokens shone.
A cheerful hop — migration's done!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: adding a UI to test progress-tracked migration in the sample/test app, which matches the core purpose of this PR.
Description check ✅ Passed The description follows the template with a linked JIRA ticket and comprehensive details about the changes, new files, modified files, and implementation approach.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch SDKS-4915

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt (2)

71-73: Expose StateFlow instead of a mutable MutableStateFlow from the ViewModel API.

Line 71 currently allows external mutation of UI state. Prefer private mutable backing field + public immutable flow.

Proposed refactor
-class AuthMigrationViewModel : ViewModel() {
-
-    var state = MutableStateFlow(AuthMigrationState())
-        private set
+class AuthMigrationViewModel : ViewModel() {
+
+    private val _state = MutableStateFlow(AuthMigrationState())
+    val state = _state
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt`
around lines 71 - 73, The ViewModel currently exposes a mutable MutableStateFlow
named "state" allowing external mutation; change it to a private mutable backing
field and a public immutable StateFlow by renaming the mutable to something like
"_state" (private val _state = MutableStateFlow(AuthMigrationState())) and
exposing public val state: StateFlow<AuthMigrationState> = _state.asStateFlow()
(or _state as StateFlow) so consumers cannot modify the flow; update any
internal mutations to use _state.value and add the necessary import for
asStateFlow if used.

124-143: Avoid appending duplicate step rows for repeated InProgress events.

Line 126 appends a new StepResult each time progress is emitted for the same step. Update existing step entries by description before adding new ones.

Proposed fix
                             is MigrationProgress.InProgress -> {
                                 state.update { currentState ->
-                                    val updatedSteps = currentState.stepResults + StepResult(
-                                        description = progress.step.description,
-                                        status = StepStatus.IN_PROGRESS
-                                    )
+                                    val existingIndex = currentState.stepResults.indexOfFirst {
+                                        it.description == progress.step.description
+                                    }
+                                    val updatedSteps = if (existingIndex >= 0) {
+                                        currentState.stepResults.toMutableList().apply {
+                                            this[existingIndex] = this[existingIndex].copy(status = StepStatus.IN_PROGRESS)
+                                        }
+                                    } else {
+                                        currentState.stepResults + StepResult(
+                                            description = progress.step.description,
+                                            status = StepStatus.IN_PROGRESS
+                                        )
+                                    }
                                     currentState.copy(stepResults = updatedSteps)
                                 }
                             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt`
around lines 124 - 143, The InProgress handler currently always appends a new
StepResult causing duplicates; change the MigrationProgress.InProgress branch
(inside state.update) to first search currentState.stepResults for an entry with
description == progress.step.description and if found map/replace that entry
with step.copy(status = StepStatus.IN_PROGRESS), otherwise append a new
StepResult; keep using state.update and the StepResult/stepResults collection so
the logic mirrors the StepCompleted branch but sets status to IN_PROGRESS when
updating an existing step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationScreen.kt`:
- Around line 334-340: The Start Migration button is still enabled when no
legacy data exists; update the disabled logic so it also checks the
migration-needed flag: modify the isDisabled computation (currently using
state.migrationStatus and MigrationStatus.RUNNING/COMPLETED/CHECKING) to include
!state.isMigrationNeeded (or state.isMigrationNeeded == false), and keep the
Button(onClick = onClick, enabled = !isDisabled) usage unchanged so the button
is disabled when migration isn't needed.

In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt`:
- Around line 78-83: The migration-required check runs on the main dispatcher
and can block the UI; wrap the call to
DefaultStorageClientProvider(context).isMigrationRequired(context) in
withContext(Dispatchers.IO) inside the existing viewModelScope.launch so the
blocking SharedPreferences access happens off the main thread; keep updating
state (MigrationStatus.CHECKING) before the switch and preserve the try/catch
flow around isMigrationRequired to update migrationStatus based on the result or
error.

In `@samples/pingsampleapp/src/main/res/values/strings.xml`:
- Around line 236-238: The migration instructions currently assert
SharedPreferences persist across uninstall/reinstall which is misleading; update
the strings auth_migration_instruction_4, auth_migration_instruction_5 and
auth_migration_instructions_note to either (a) instruct testers to perform an
app update/replace-install (not uninstall/reinstall) to preserve app-private
data, or (b) explicitly state that persistence across uninstall/reinstall
depends on Android Auto Backup being enabled and is not guaranteed; pick one
approach and make the wording precise so testers know the required install path
or the backup dependency.

---

Nitpick comments:
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt`:
- Around line 71-73: The ViewModel currently exposes a mutable MutableStateFlow
named "state" allowing external mutation; change it to a private mutable backing
field and a public immutable StateFlow by renaming the mutable to something like
"_state" (private val _state = MutableStateFlow(AuthMigrationState())) and
exposing public val state: StateFlow<AuthMigrationState> = _state.asStateFlow()
(or _state as StateFlow) so consumers cannot modify the flow; update any
internal mutations to use _state.value and add the necessary import for
asStateFlow if used.
- Around line 124-143: The InProgress handler currently always appends a new
StepResult causing duplicates; change the MigrationProgress.InProgress branch
(inside state.update) to first search currentState.stepResults for an entry with
description == progress.step.description and if found map/replace that entry
with step.copy(status = StepStatus.IN_PROGRESS), otherwise append a new
StepResult; keep using state.update and the StepResult/stepResults collection so
the logic mirrors the StepCompleted branch but sets status to IN_PROGRESS when
updating an existing step.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c46740e9-afeb-4d5e-a772-03b9a74c39f1

📥 Commits

Reviewing files that changed from the base of the PR and between 4e2f953 and 4117507.

📒 Files selected for processing (6)
  • samples/pingsampleapp/build.gradle.kts
  • samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationScreen.kt
  • samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt
  • samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/home/HomeApp.kt
  • samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/navigation/Navigation.kt
  • samples/pingsampleapp/src/main/res/values/strings.xml

Comment on lines +334 to +340
val isDisabled = state.migrationStatus == MigrationStatus.RUNNING
|| state.migrationStatus == MigrationStatus.COMPLETED
|| state.migrationStatus == MigrationStatus.CHECKING

Button(
onClick = onClick,
enabled = !isDisabled,
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Disable Start Migration when no legacy data is detected.

Currently, after status shows “None”, the button can still be pressed. Consider disabling when isMigrationNeeded == false to prevent unnecessary runs and reduce confusion.

Proposed fix
 private fun StartMigrationButton(
     state: AuthMigrationState,
     onClick: () -> Unit
 ) {
     val isDisabled = state.migrationStatus == MigrationStatus.RUNNING
             || state.migrationStatus == MigrationStatus.COMPLETED
             || state.migrationStatus == MigrationStatus.CHECKING
+            || state.isMigrationNeeded == false
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
val isDisabled = state.migrationStatus == MigrationStatus.RUNNING
|| state.migrationStatus == MigrationStatus.COMPLETED
|| state.migrationStatus == MigrationStatus.CHECKING
Button(
onClick = onClick,
enabled = !isDisabled,
val isDisabled = state.migrationStatus == MigrationStatus.RUNNING
|| state.migrationStatus == MigrationStatus.COMPLETED
|| state.migrationStatus == MigrationStatus.CHECKING
|| state.isMigrationNeeded == false
Button(
onClick = onClick,
enabled = !isDisabled,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationScreen.kt`
around lines 334 - 340, The Start Migration button is still enabled when no
legacy data exists; update the disabled logic so it also checks the
migration-needed flag: modify the isDisabled computation (currently using
state.migrationStatus and MigrationStatus.RUNNING/COMPLETED/CHECKING) to include
!state.isMigrationNeeded (or state.isMigrationNeeded == false), and keep the
Button(onClick = onClick, enabled = !isDisabled) usage unchanged so the button
is disabled when migration isn't needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is intended state to keep the button enabled.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vibhorgoswami, understood — keeping the button enabled regardless of migration status is intentional. I'll defer to that design decision. Thanks for the clarification!

(´• ω •`) 🐇


✏️ Learnings added
Learnt from: vibhorgoswami
Repo: ForgeRock/ping-android-sdk PR: 180
File: samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationScreen.kt:334-340
Timestamp: 2026-04-09T15:11:28.724Z
Learning: In `samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationScreen.kt`, the Start Migration button is intentionally kept enabled even when `isMigrationNeeded == false` (no legacy data detected). Do not suggest disabling the button based on the migration-needed flag.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment on lines +236 to +238
<string name="auth_migration_instruction_4">Install this sample app (same application ID ensures SharedPreferences data persists).</string>
<string name="auth_migration_instruction_5">Open this screen and tap \"Start Migration\" to import the legacy credentials.</string>
<string name="auth_migration_instructions_note">Note: Only one app with the same application ID can be installed at a time. SharedPreferences data persists across app installs/uninstalls as long as the application ID matches.</string>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Does Android preserve SharedPreferences data after app uninstall and reinstall for the same application ID?

💡 Result:

Normally, no: when an Android app is uninstalled, the app’s private data directory is removed, which includes its SharedPreferences.

However, it can appear preserved after reinstall (same application ID/package name) because Android’s backup/restore can restore that data:

  • On Android 6.0+, Auto Backup (Google Drive) can back up app-private data including SharedPreferences, and restore it when the app is reinstalled, as long as backups are enabled and your app participates in backup. (developer.android.com)
  • By default, an app participates because android:allowBackup defaults to true (unless you set it to false). (developer.android.com)

So the correct rule is: Uninstall clears local SharedPreferences, but backup mechanisms may restore them on reinstall, so you must not rely on uninstall/reinstall to guarantee a “clean slate” unless you disable/limit backup. (developer.android.com)

Citations:


Migration test instructions are misleading about SharedPreferences persistence.

Lines 236–238 claim SharedPreferences data persists across uninstall/reinstall, but this is only true if Android's Auto Backup mechanism is enabled and functioning. On a plain uninstall/reinstall, SharedPreferences data is normally deleted; the app's private directory is removed. To avoid developer confusion during testing, either:

  • Use an app update/replace-install path instead of uninstall/reinstall, or
  • Explicitly document that this flow depends on Android Auto Backup being enabled (default behavior on Android 6.0+) and that data restoration is not guaranteed without a working backup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@samples/pingsampleapp/src/main/res/values/strings.xml` around lines 236 -
238, The migration instructions currently assert SharedPreferences persist
across uninstall/reinstall which is misleading; update the strings
auth_migration_instruction_4, auth_migration_instruction_5 and
auth_migration_instructions_note to either (a) instruct testers to perform an
app update/replace-install (not uninstall/reinstall) to preserve app-private
data, or (b) explicitly state that persistence across uninstall/reinstall
depends on Android Auto Backup being enabled and is not guaranteed; pick one
approach and make the wording precise so testers know the required install path
or the backup dependency.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manifest file shows that Auto Backup is enabled under allowBackup=true and the API level is 29 which makes it Android 10.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

Thanks for the context! Let me verify the manifest details quickly.


🏁 Script executed:

#!/bin/bash
# Find the AndroidManifest.xml for the pingsampleapp
fd -i "AndroidManifest.xml" --search-path samples/pingsampleapp --exec cat {}

Length of output: 5233


@vibhorgoswami, confirmed — the manifest does have android:allowBackup="true" with no backup exclusion rules (fullBackupContent or dataExtractionRules), which means SharedPreferences are included in Auto Backup by default. On Android 10 (API 29), Auto Backup (Google Drive) can back up app-private data including SharedPreferences and restore it when the app is reinstalled.

That does substantially mitigate the concern. The one residual nuance is that Auto Backup still requires:

  • A Google account signed in on the test device
  • Device backup enabled in Android Settings → Google → Backup

Since this is a developer-facing test screen (not end-user facing), the current wording is reasonable. You may optionally add a small note to auth_migration_instructions_note mentioning the Auto Backup dependency — something like "…as long as Android Auto Backup is enabled on the device" — but I'll leave that call to the PR author. The concern is largely addressed given the explicit allowBackup=true and no exclusion rules in the manifest.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.67%. Comparing base (4e2f953) to head (8277764).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@            Coverage Diff             @@
##             develop     #180   +/-   ##
==========================================
  Coverage      46.67%   46.67%           
  Complexity      1263     1263           
==========================================
  Files            306      306           
  Lines           9215     9215           
  Branches        1305     1305           
==========================================
  Hits            4301     4301           
  Misses          4347     4347           
  Partials         567      567           
Flag Coverage Δ
integration-tests 28.31% <ø> (-0.02%) ⬇️
unit-tests 27.66% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@vibhorgoswami vibhorgoswami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments on the auto review but otherwise LGTM!

Comment on lines +236 to +238
<string name="auth_migration_instruction_4">Install this sample app (same application ID ensures SharedPreferences data persists).</string>
<string name="auth_migration_instruction_5">Open this screen and tap \"Start Migration\" to import the legacy credentials.</string>
<string name="auth_migration_instructions_note">Note: Only one app with the same application ID can be installed at a time. SharedPreferences data persists across app installs/uninstalls as long as the application ID matches.</string>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manifest file shows that Auto Backup is enabled under allowBackup=true and the API level is 29 which makes it Android 10.

Comment on lines +334 to +340
val isDisabled = state.migrationStatus == MigrationStatus.RUNNING
|| state.migrationStatus == MigrationStatus.COMPLETED
|| state.migrationStatus == MigrationStatus.CHECKING

Button(
onClick = onClick,
enabled = !isDisabled,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is intended state to keep the button enabled.

…ingsampleapp/authmigration/AuthMigrationViewModel.kt

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Rodrigo Reis <rodrigoareis@gmail.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt`:
- Around line 77-107: The function checkMigrationNeeded contains misplaced
import statements (Dispatchers, withContext, FlowCollector, MutableStateFlow,
update) inside its body causing a syntax error; remove those import lines from
inside the function and add the required imports (at least
kotlinx.coroutines.Dispatchers and kotlinx.coroutines.withContext) at the top of
the file with the other imports so checkMigrationNeeded can use Dispatchers.IO
and withContext when calling
DefaultStorageClientProvider(context.applicationContext). Ensure no duplicate
imports remain and keep the function body starting with viewModelScope.launch as
originally intended.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5a375d2d-664d-4c4c-9dcb-a978283078a2

📥 Commits

Reviewing files that changed from the base of the PR and between 4117507 and 8946144.

📒 Files selected for processing (1)
  • samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt (1)

73-74: Encapsulate mutable state (MutableStateFlow) inside the ViewModel.

state is currently exposed as MutableStateFlow, allowing external code to call mutating APIs directly (.update(), .emit(), etc.) despite the private set. Expose immutable StateFlow instead and keep the mutable backing property private.

♻️ Proposed refactor
 import kotlinx.coroutines.flow.FlowCollector
 import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.StateFlow
+import kotlinx.coroutines.flow.asStateFlow
 import kotlinx.coroutines.flow.update
 ...
-    var state = MutableStateFlow(AuthMigrationState())
-        private set
+    private val _state = MutableStateFlow(AuthMigrationState())
+    val state: StateFlow<AuthMigrationState> = _state.asStateFlow()

Replace internal writes from state.update { ... } to _state.update { ... } and reads like state.value to _state.value.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt`
around lines 73 - 74, The public mutable StateFlow should be encapsulated:
change the exposed property `state` of type MutableStateFlow to an immutable
StateFlow by creating a private backing `MutableStateFlow` (e.g., private val
_state = MutableStateFlow(AuthMigrationState())) and expose public val state:
StateFlow<AuthMigrationState> = _state; then update all internal mutations to
call `_state.update { ... }` (or `_state.emit` if used) and internal reads that
referenced `state.value` to `_state.value`, keeping external access read-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt`:
- Around line 93-99: In the catch block inside AuthMigrationViewModel where
state.update currently sets migrationStatus = MigrationStatus.IDLE and
isMigrationNeeded = false, do not convert exceptions into "no migration needed";
instead set migrationStatus to a dedicated failure state (e.g.,
MigrationStatus.CHECK_FAILED — add this enum value if it doesn't exist) and do
not force isMigrationNeeded = false (leave it unchanged or null), and log the
exception so the failed check is visible; update the state.update call and add
exception logging accordingly.

---

Nitpick comments:
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt`:
- Around line 73-74: The public mutable StateFlow should be encapsulated: change
the exposed property `state` of type MutableStateFlow to an immutable StateFlow
by creating a private backing `MutableStateFlow` (e.g., private val _state =
MutableStateFlow(AuthMigrationState())) and expose public val state:
StateFlow<AuthMigrationState> = _state; then update all internal mutations to
call `_state.update { ... }` (or `_state.emit` if used) and internal reads that
referenced `state.value` to `_state.value`, keeping external access read-only.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0d396368-727f-450e-b820-081a8e072d2d

📥 Commits

Reviewing files that changed from the base of the PR and between 8946144 and 8277764.

📒 Files selected for processing (1)
  • samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt

Comment on lines +93 to +99
} catch (_: Exception) {
state.update {
it.copy(
migrationStatus = MigrationStatus.IDLE,
isMigrationNeeded = false
)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don’t convert check failures into “no migration needed.”

On Line 97, exceptions are mapped to isMigrationNeeded = false, which hides real check failures and can mislead test results. Prefer surfacing this as a failed check state.

🐛 Proposed fix
-            } catch (_: Exception) {
+            } catch (e: Exception) {
                 state.update {
                     it.copy(
-                        migrationStatus = MigrationStatus.IDLE,
-                        isMigrationNeeded = false
+                        migrationStatus = MigrationStatus.FAILED,
+                        isMigrationNeeded = null,
+                        errorMessage = e.message ?: "Failed to check migration status."
                     )
                 }
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authmigration/AuthMigrationViewModel.kt`
around lines 93 - 99, In the catch block inside AuthMigrationViewModel where
state.update currently sets migrationStatus = MigrationStatus.IDLE and
isMigrationNeeded = false, do not convert exceptions into "no migration needed";
instead set migrationStatus to a dedicated failure state (e.g.,
MigrationStatus.CHECK_FAILED — add this enum value if it doesn't exist) and do
not force isMigrationNeeded = false (leave it unchanged or null), and log the
exception so the failed check is visible; update the state.update call and add
exception logging accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants