refactor(migration): simplify - #72
Merged
Merged
Conversation
The loading tail writes the scope's state back when the block returns, so the Migrating submit path's direct write to _uiState was overwritten and a rejected v1 main password stopped the spinner without marking the field. Route it through the scope, the way the Login path already does. Retry is a button the user reaches after a failure, so it can be tapped twice before the first run publishes anything. Two concurrent imports would read and write the same v1 rows, so hold the job and drop the second tap.
Drives the real Submit event, which needs a hash ValidateMainPasswordUseCase can hex-decode. The other tests seed a placeholder and enter past validation.
The migrate path nested one loading inside another: the outer block returned as soon as executeCreateAccess had launched, so loading went back to false while key derivation was still running and Submit re-enabled for the whole of it. onEvent gates on the state being interactable rather than on the loading flag, so nothing below the button stopped a second run either. Two runs mint two accounts, two ARKs and two vaults, and the second overwrites the registry, leaving the first vault wrapped under an ARK nothing persists. Flatten the migrate path onto the caller's loading scope, and hold the job so a run that lands while another is in flight is dropped.
…ists The flattening had no coverage: the existing test drives executeCreateAccess, whose only production caller is the biometric callback, not the Submit path the defect was on. The two halves of the fix are coupled in a non-obvious direction. With the guard in place but the nesting still there, the inner loading is the run the guard drops, so no account is created at all. Verified by restoring the nesting: this test then fails. Also names the real reason the guard cannot eat the biometric hand-off. The collector resumes inline inside trySend, so AuthScreen runs while the job is still active; what saves it is requestCipher suspending.
OffRange
commented
Aug 16, 2026
OffRange
left a comment
OffRange
left a comment
Owner
Author
There was a problem hiding this comment.
Review of the migration consolidation. Most of this reads as a clean simplification; the notes below are the places where behaviour changed in ways I do not think were intended. The RunPendingMigrationUseCase one is the only blocker in my view.
One cross-PR note that has no line to hang on here: #73 edits migration/legacy-data/.../LegacyItemConverter.kt to pass the now-mandatory passkeyRPs, while this PR moves that file to legacy-migration/. Git resolves rename-vs-edit in favor of this PR's content, so whichever of the two merges second will fail to compile with No value passed for parameter passkeyRPs. Worth agreeing a merge order, and re-checking any other Login(...) construction both branches touch.
The Migrated branch cleared the marker unconditionally, but the import returns Migrated with fileRetained = true whenever the prune failed or the recount came back non-zero. On that path the marker went and Completed(skippedItems = 0) sent the user straight into the app. hasMainPassword() is false from then on, so nothing ever deletes secure_element_database or the password_manager_skey alias again: the v1 database stays on disk, still decryptable by a key still in the Keystore, with no path that retries. Gate the clear on LegacyMigrationOutcome.nothingLeftToImport, which is what it was added for and until now was read nowhere. A retained file keeps the marker and the next unlock tries again. The trade this reverses is a prune that keeps failing over rows already in v2, which reimports them each unlock. A duplicate is visible and undoable; a v1 database nothing can reach is neither. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Moving the import onto viewModelScope changed two things beyond where the job is held. It put the work on the main thread. viewModelScope is Dispatchers.Main.immediate and nothing below it switches: LegacyKeyRepositoryImpl reaches the Keystore, LegacyAesGcmCipher decrypts per row, converter.convert re-encrypts every field per item, and deleteDatabaseAndKey is not even suspending. On a vault of a few hundred items that is seconds of StrictMode-violating work on the auth screen, and an ANR at the top end. It also opened a cancellation window that did not exist before. The batch write commits before the prune runs, so a back press between the two - or a popped nav entry, or a destroyed activity - cancelled the run. Cancellation is rethrown all the way up on purpose, so the marker stayed set over rows already in v2 and the next unlock imported them again under fresh ids: two copies of every v1 item. On the old application-scoped job only process death could do that. RunPendingMigrationUseCase owns the run instead. It is already the @single that sequences the migration, so the scope and the one-run-at-a-time state have somewhere to live without a class in between: it starts the run on an application-lived IO scope and hands out a join on it. The ViewModel keeps the states it grew - ImportingLegacyData, the summary, the failure - but what its scope cancels is the waiting, not the import. Callers that overlap share one run and one verdict, which is also how a screen destroyed mid-import and rebuilt still reports the summary for a run it did not start. A run that has already finished is not replayed, so Retry after a failure really does retry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The update that ends a loading run writes back a snapshot taken before block() suspended, but lost the `if (it !is AuthState.Interactable) return@update it` guard that the update starting the run still has. onSessionEstablished is reachable from outside loading: AuthScreen calls it straight from the BiometricRequest.Login success handler, and nothing gates it on authJob. So the user submits the password form, the biometric prompt they had already triggered succeeds, the import starts and puts the screen in ImportingLegacyData, and this line then drops the stale Login snapshot back over it. The import is left running invisibly behind a live login form that can be submitted again. Guard it the same way, and pin it: the test fails against the unguarded write. FakeAccountRepository grows a held-open account read because it is the only place a test can stand inside an unlock - everything after it hops to Dispatchers.Default, which the test scheduler cannot see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One Log.e call in AuthViewModel had turned returnDefaultValues on for every unit test in the module. The cost is module-wide and open-ended: any future test here that touches an unmocked android.jar method - TextUtils.isEmpty, Uri.parse, Base64 - silently gets 0/null/false and passes against behaviour that would crash on device, instead of failing loudly. MigrationResult.Incomplete already carries the cause, so AuthState.MigrationFailed carries it too and AuthScreen writes it out. The record survives, the branch is now assertable on state rather than on a logger, and the flag goes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ove it
The import's scope carried Dispatchers.IO, which made every suspend function
under it main-safe by accident rather than by contract. A caller that did not
know to pick that scope - the autofill service, a future worker, a test - got
whatever thread it came in on, and the module's own signatures gave it no
reason to expect otherwise.
Push the switch down to the calls that actually block:
- LegacyKeyRepository.secretKey/deleteLegacyKey become suspend on IO. Loading
the Keystore reaches the keymaster HAL.
- LegacyItemRepository.deleteDatabase becomes suspend on IO, and withDao runs
there too. Room moves its own suspending queries, but databaseProvider.get()
stats the path and builds the database, and the first query through it runs
v1's 1/2-to-3 recreate. The provider is @synchronized and cannot itself be
suspend, so the repository is as low as this goes.
- readAll's row loop runs on Default. An AES-GCM open and a JSON parse per row
is processor work, and it is the part that scales with the old vault.
The re-encryption was already main-safe through CryptographicScopeImpl, so it
needed nothing.
provideMigrationScope now names no dispatcher and carries only the SupervisorJob
and the application lifetime, which is the part no withContext can substitute
for. Pinning a dispatcher there as well would only hide the next call that
forgets to switch.
Behaviour is unchanged. Pinned by a test that fails if any of the repository's
switches is dropped, which nothing else would catch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OffRange
added a commit
that referenced
this pull request
Aug 19, 2026
#72 moved LegacyItemConverter.kt to legacy-migration and its copy of the Login(...) call had no passkeys argument, so whichever of the two branches merged second would fail to compile now that the default is gone from Login. #72 landed first, so the argument is carried over onto its relocated file here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.