Merged
Conversation
📝 WalkthroughWalkthroughReplaces direct bootstrap with a watchdoged, cancellable bootstrap across platforms; Rust exposes stepwise bootstrap state, active migration progress and cancellation APIs; mobile UIs show adaptive timeouts, migration/encryption status/progress, and kick off non-blocking initData after completion. Changes
Sequence DiagramsequenceDiagram
participant UI as Mobile UI
participant Runner as TaskGroup / Coroutine Runner
participant WD as Watchdog
participant RustBoot as Rust bootstrap()
participant RustMig as Rust migration APIs
participant AppMgr as AppManager / App instance
participant InitData as Rust initData()
UI->>Runner: start boot sequence
Runner->>RustBoot: call bootstrap()
WD->>RustMig: poll active_migration().progress() (≈66ms)
alt migration detected
WD-->>UI: update statusMessage & progress
WD->>Runner: extend timeout
end
par bootstrap completes
RustBoot-->>Runner: success (optional warning)
Runner->>AppMgr: initialize managers
AppMgr->>InitData: trigger initData() (async, fire-and-forget)
Runner-->>WD: cancel watchdog
and timeout occurs first
WD->>RustBoot: call cancel_bootstrap()
Runner-->>UI: surface bootstrapError or last-step completion
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
praveenperera
force-pushed
the
fix-splash-stall
branch
from
1d7af8b to
5e98aad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ios/Cove/CoveApp.swift (2)
32-36: Consider movingpublicto the extension declaration.The swiftformat linter flags the access control placement. Moving
publicto the extension level is more idiomatic.♻️ Suggested refactor
-extension EnvironmentValues { - public var safeAreaInsets: EdgeInsets { +public extension EnvironmentValues { + var safeAreaInsets: EdgeInsets { self[SafeAreaInsetsKey.self] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoveApp.swift` around lines 32 - 36, The access control for safeAreaInsets should be moved to the extension declaration: change the declaration on the extension for EnvironmentValues to be public (i.e. make the extension public) and remove the public modifier from the computed property safeAreaInsets so the extension-level access control applies; update the extension EnvironmentValues and ensure SafeAreaInsetsKey.self usage remains unchanged.
64-74: Good timeout pattern, but consider avoiding the force unwrap.The
TaskGrouprace pattern is a solid approach for the 30-second timeout. However,group.next()!uses a force unwrap which, while safe here (tasks are added beforenext()is called), is not idiomatic Swift.♻️ Suggested safer alternative
- let result = try await group.next()! + guard let result = try await group.next() else { + throw BootstrapTimeoutError() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoveApp.swift` around lines 64 - 74, The current withThrowingTaskGroup uses a force unwrap on group.next() (let result = try await group.next()!) which is unsafe/uncharted; change it to safely unwrap the optional return from group.next() (e.g., use guard let or if let to bind the value) and handle the nil case by throwing an appropriate error (reuse BootstrapTimeoutError or a new descriptive error) before calling group.cancelAll(), keeping the rest of the race logic with bootstrap() and the 30s timeout intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ios/Cove/CoveApp.swift`:
- Around line 32-36: The access control for safeAreaInsets should be moved to
the extension declaration: change the declaration on the extension for
EnvironmentValues to be public (i.e. make the extension public) and remove the
public modifier from the computed property safeAreaInsets so the extension-level
access control applies; update the extension EnvironmentValues and ensure
SafeAreaInsetsKey.self usage remains unchanged.
- Around line 64-74: The current withThrowingTaskGroup uses a force unwrap on
group.next() (let result = try await group.next()!) which is unsafe/uncharted;
change it to safely unwrap the optional return from group.next() (e.g., use
guard let or if let to bind the value) and handle the nil case by throwing an
appropriate error (reuse BootstrapTimeoutError or a new descriptive error)
before calling group.cancelAll(), keeping the rest of the race logic with
bootstrap() and the 30s timeout intact.
Contributor
Author
|
@greptile-apps re-review |
Additional Comments (1)
The PR description states: "Add slow-startup feedback ('Taking longer than expected...') to the splash screen after 10 seconds." However, This could be intentional (deferred to a follow-up), an oversight, or inaccuracy in the PR description. Please confirm whether this feature should be included in this PR or if it was intentionally deferred. |
praveenperera
force-pushed
the
fix-splash-stall
branch
from
d1de656 to
e10b2f2
Compare
Contributor
Author
|
@greptile-apps re-review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Author
|
@greptile-apps re-review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ios/Cove/CoveApp.swift (3)
82-89: Include the last bootstrap step in the visible error text.You already fetch
bootstrapProgress(). Surfacing it inbootstrapErrorwill materially improve user support diagnostics.💡 Suggested fix
} catch is BootstrapTimeoutError { let step = bootstrapProgress() Log.error("[STARTUP] bootstrap timed out after 30s, last step: \(step)") bootstrapError = - "App startup timed out. Please force-quit and try again.\n\nPlease contact feedback@covebitcoinwallet.com" + "App startup timed out. Last step: \(step.isEmpty ? "unknown" : step).\nPlease force-quit and try again.\n\nPlease contact feedback@covebitcoinwallet.com" } catch { let step = bootstrapProgress() Log.error("[STARTUP] bootstrap failed at step: \(step), error: \(error)") - bootstrapError = error.localizedDescription + bootstrapError = "Startup failed at step: \(step.isEmpty ? "unknown" : step).\n\(error.localizedDescription)" }As per coding guidelines "ios/Cove/**/*.swift: Review SwiftUI view code for proper layout, best practices."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoveApp.swift` around lines 82 - 89, Include the last bootstrap step text in the user-visible error by appending the value from bootstrapProgress() to bootstrapError in both the timeout and catch branches: when creating bootstrapError in the timeout block (where you call bootstrapProgress() into step and log it) and in the catch block (where you already log step and error), build bootstrapError to include step (e.g., "Last step: \(step)") along with the existing message or error.localizedDescription so the UI shows the last bootstrap step for diagnostics.
80-80: UseTask.detached(priority: .background)for post-bootstrapinitData()initialization.Since
initData()is non-critical background work after app bootstrap, detached execution avoids inheriting MainActor context from the.taskmodifier, reducing unnecessary actor-affinity overhead. Prefer the localappManagerreference rather than accessingAppManager.sharedagain.💡 Suggested fix
- Task { await appManager.rust.initData() } + Task.detached(priority: .background) { + await appManager.rust.initData() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoveApp.swift` at line 80, The current Task { await appManager.rust.initData() } runs on the caller's actor and may inherit MainActor; change it to use Task.detached(priority: .background) to run initData() as non-critical background work and avoid MainActor affinity, and reference the existing local appManager instance (do not call AppManager.shared inside the task) so the detached task invokes await appManager.rust.initData() with background priority.
69-70: Avoid force-unwrappinggroup.next()result.
group.next()returns an optional; in this task group configuration with optional result type (String?.self), explicit handling prevents undefined behavior if task-group composition changes. Use a guard pattern instead.💡 Suggested fix
- let result = try await group.next()! + guard let result = try await group.next() else { + throw BootstrapTimeoutError() + } group.cancelAll() return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoveApp.swift` around lines 69 - 70, Replace the force-unwrap of group.next() by explicitly handling the optional result from the task group: change the usage around the variable named result (where you currently do let result = try await group.next()!) to a guard/if-let that safely unwraps the optional returned by group.next(), handle the nil case (e.g., skip processing, return, or log and cancel tasks via group.cancelAll()) and only proceed to use result when non-nil; ensure group.cancelAll() remains called as needed after handling the nil/unwrapped branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/bootstrap.rs`:
- Around line 133-143: The mutex handling for BOOTSTRAP_PROGRESS should recover
poisoned locks instead of silently dropping state: in set_progress replace the
if let Ok(mut progress) = BOOTSTRAP_PROGRESS.lock() branch to obtain the guard
via BOOTSTRAP_PROGRESS.lock().unwrap_or_else(|e| e.into_inner()) and then write
into the guard (so set_progress uses the recovered guard to call clone_into);
similarly, in bootstrap_progress replace BOOTSTRAP_PROGRESS.lock().map(|s|
s.clone()).unwrap_or_default() with BOOTSTRAP_PROGRESS.lock().map(|s|
s.clone()).unwrap_or_else(|e| e.into_inner()) (or lock().unwrap_or_else(|e|
e.into_inner()).clone()) so a poisoned mutex returns the preserved String
instead of emptying it; reference the BOOTSTRAP_PROGRESS static and the
set_progress and bootstrap_progress functions when making these changes.
---
Nitpick comments:
In `@ios/Cove/CoveApp.swift`:
- Around line 82-89: Include the last bootstrap step text in the user-visible
error by appending the value from bootstrapProgress() to bootstrapError in both
the timeout and catch branches: when creating bootstrapError in the timeout
block (where you call bootstrapProgress() into step and log it) and in the catch
block (where you already log step and error), build bootstrapError to include
step (e.g., "Last step: \(step)") along with the existing message or
error.localizedDescription so the UI shows the last bootstrap step for
diagnostics.
- Line 80: The current Task { await appManager.rust.initData() } runs on the
caller's actor and may inherit MainActor; change it to use
Task.detached(priority: .background) to run initData() as non-critical
background work and avoid MainActor affinity, and reference the existing local
appManager instance (do not call AppManager.shared inside the task) so the
detached task invokes await appManager.rust.initData() with background priority.
- Around line 69-70: Replace the force-unwrap of group.next() by explicitly
handling the optional result from the task group: change the usage around the
variable named result (where you currently do let result = try await
group.next()!) to a guard/if-let that safely unwraps the optional returned by
group.next(), handle the nil case (e.g., skip processing, return, or log and
cancel tasks via group.cancelAll()) and only proceed to use result when non-nil;
ensure group.cancelAll() remains called as needed after handling the
nil/unwrapped branch.
13e68f2 to
09eabaa
Compare
praveenperera
changed the title
praveenperera
force-pushed
the
fix-splash-stall
branch
from
069fcde to
857f043
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt`:
- Around line 226-231: The catch for BootstrapTimeoutException currently always
sets bootstrapError and aborts startup; instead, call bootstrapProgress() inside
the catch, check if it indicates completion (i.e., the bootstrap is already
finished) and if so simply continue startup (do not set bootstrapError or return
from the LaunchedEffect); only treat the timeout as fatal and set
bootstrapError/return if bootstrapProgress() shows it is not complete. Locate
this logic around the LaunchedEffect catch block that references
BootstrapTimeoutException, bootstrapProgress(), TAG and the bootstrapError
variable and implement the conditional behavior mirroring the iOS handling.
- Around line 204-220: The loop currently only updates splashStatus when
bootstrapMigrationProgress() returns a migration, so slow non-migration boots
never show a “taking longer than expected” message; inside the while
(bootstrapDeferred.isActive) loop, after calling bootstrapMigrationProgress()
and handling the migration branch, add an else branch that sets splashStatus =
"Taking longer than expected..." (and clear or set encryptionProgress = null/0f
as appropriate) once elapsed crosses a shorter threshold (e.g., after a few
seconds) so users receive slow-start feedback; keep cancelBootstrap() and the
BootstrapTimeoutException() behavior unchanged.
In `@ios/Cove/CoverView.swift`:
- Around line 86-97: The loop in the Task only sets statusMessage when
bootstrapMigrationProgress() returns a migration, so slow non-migration startups
never show the “Taking longer than expected...” text; update the .task block
around the while loop that calls bootstrapMigrationProgress() to record a start
time (e.g., start = Date()) and, when no migration is present
(bootstrapMigrationProgress() is nil or migration.total == 0) and elapsed time
exceeds a short threshold (e.g., 2s), set statusMessage = "Taking longer than
expected..." and clear or set encryptionProgress to nil/0 as appropriate, while
preserving the existing migration branch that sets "Encrypting data..." and
encryptionProgress when migration is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8dbfc42c-f625-40de-991d-ae6c48ea1fbf
📒 Files selected for processing (7)
android/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktios/Cove/CoveApp.swiftios/Cove/CoverView.swiftrust/src/bootstrap.rsrust/src/database/migration.rsrust/src/database/migration/v1_bdk.rsrust/src/database/migration/v1_redb.rs
praveenperera
force-pushed
the
fix-splash-stall
branch
from
daaef5f to
c1c5a27
Compare
praveenperera
force-pushed
the
fix-splash-stall
branch
11 times, most recently
from
5ec1da4 to
eb2acd1
Compare
Greptile SummaryThis PR fixes cold-launch splash stalls by removing Key findings from the review:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS/Android UI
participant Watchdog as bootstrapWatchdog
participant FFI as UniFFI / Rust
participant Bootstrap as bootstrap() (tokio)
participant InitData as initData() (detached)
UI->>FFI: bootstrap()
activate Bootstrap
Bootstrap->>Bootstrap: set_step(Initializing)
Bootstrap->>Bootstrap: init tokio, derive encryption key
Bootstrap->>Bootstrap: set_step(MigratingMainDatabase)
Bootstrap->>Bootstrap: run redb migrations
par Watchdog polling (every 66 ms)
UI->>Watchdog: start bootstrapWatchdog()
loop while bootstrap active
Watchdog->>FFI: bootstrapProgress()
FFI-->>Watchdog: BootstrapStep
Watchdog->>FFI: activeMigration()?.progress()
FFI-->>Watchdog: MigrationProgress (current/total)
Watchdog->>UI: update progress bar / status message
alt timeout exceeded
Watchdog->>FFI: cancelBootstrap()
Watchdog-->>UI: BootstrapTimeoutError / timedOut
end
end
end
Bootstrap->>Bootstrap: set_step(MigratingBdkDatabases)
Bootstrap->>Bootstrap: attempt BDK migration
Bootstrap->>Bootstrap: set_step(Complete)
Bootstrap-->>UI: Ok(warning?)
deactivate Bootstrap
UI->>UI: completeBootstrap()
UI->>InitData: Task { initData() } [detached, fire-and-forget]
activate InitData
InitData-->>UI: (background — no await)
deactivate InitData
|
praveenperera
force-pushed
the
fix-splash-stall
branch
from
eb2acd1 to
bc9f3ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (2)
47096-47106: Clarify ownership expectations foractiveMigration()return value.Consider explicitly documenting that callers should
close()/destroy()the returnedMigrationhandle when done, especially in polling flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt` around lines 47096 - 47106, Document ownership for activeMigration(): update the KDoc on the activeMigration() function to explicitly state that the returned Migration is a native/FFI handle owned by the caller and must be closed/destroyed (e.g., call close() or destroy()) when no longer needed, especially in polling loops; mention that failure to do so leaks native resources and show the recommended usage pattern (poll -> close) and lifetime expectations for callers interacting with Migration objects from UniffiLib.
29516-29551: Consider adding explicit validation for BootstrapStep ordinal mapping.
FfiConverterTypeBootstrapStepcurrently depends on ordinal position (+1/-1), and enum orders between Rust and Kotlin are presently aligned. However, to prevent silent mis-mapping if variant ordering ever diverges in the future, consider adding validation that explicitly maps variant names rather than relying on ordinal position, or add a test that asserts alignment between Rust and Kotlin enum orders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt` around lines 29516 - 29551, The FfiConverterTypeBootstrapStep read/write rely on ordinal math (value.ordinal + 1 / buf.getInt() - 1) which can silently break if Rust and Kotlin enum orders diverge; update FfiConverterTypeBootstrapStep to validate/match by explicit variant names instead of ordinals (e.g., map integer/strings to BootstrapStep by name or maintain a stable explicit Int->BootstrapStep map) and add runtime checks in read() to reject out-of-range values with a clear message, and/or add a unit test that asserts the Kotlin BootstrapStep.name order matches the expected Rust names to prevent future drift; target the read and write methods of FfiConverterTypeBootstrapStep and the BootstrapStep enum for these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/database/migration/v1_bdk.rs`:
- Around line 28-60: The code currently treats any rusqlite failures as
Ok(false) (corruption); change it so only a non-"ok" integrity_check result
returns Ok(false) and all operational failures propagate as Err. Specifically,
in the rusqlite::Connection::open match, on Err(e) return Err(e.into()) (or map
to an appropriate std::io::Error) instead of Ok(false); do the same for failures
from conn.pragma_update(...) and the initial conn.query_row(...) schema check;
for the PRAGMA integrity_check match keep Ok(ref result) if result == "ok" =>
Ok(true) and Ok(result) (non-"ok") => Ok(false), but change the Err(e) arm of
the integrity_check to return Err(e.into()) so transient I/O/sql errors bubble
up to the caller (preserving path_display context in the error message where
appropriate).
In `@rust/src/database/migration/v1_redb.rs`:
- Around line 159-186: The verification function must stop masking operational
I/O errors as Ok(false); change its signature to propagate errors (e.g., return
Result<(), E> or Result<bool, E> with Err for I/O) and replace the match
branches that currently return Ok(false) on failures from
EncryptedBackend::open, redb::Database::builder().create_with_backend,
db.begin_read, and read_txn.list_tables so they return Err(wrap the original
error) instead; only return Ok(false) for explicit, deterministic corruption
detection cases. Update all callers to handle the new Err propagation (treat Err
as transient I/O/operational failures and only perform restore on a true
corruption signal).
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 47096-47106: Document ownership for activeMigration(): update the
KDoc on the activeMigration() function to explicitly state that the returned
Migration is a native/FFI handle owned by the caller and must be
closed/destroyed (e.g., call close() or destroy()) when no longer needed,
especially in polling loops; mention that failure to do so leaks native
resources and show the recommended usage pattern (poll -> close) and lifetime
expectations for callers interacting with Migration objects from UniffiLib.
- Around line 29516-29551: The FfiConverterTypeBootstrapStep read/write rely on
ordinal math (value.ordinal + 1 / buf.getInt() - 1) which can silently break if
Rust and Kotlin enum orders diverge; update FfiConverterTypeBootstrapStep to
validate/match by explicit variant names instead of ordinals (e.g., map
integer/strings to BootstrapStep by name or maintain a stable explicit
Int->BootstrapStep map) and add runtime checks in read() to reject out-of-range
values with a clear message, and/or add a unit test that asserts the Kotlin
BootstrapStep.name order matches the expected Rust names to prevent future
drift; target the read and write methods of FfiConverterTypeBootstrapStep and
the BootstrapStep enum for these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5366e36e-688b-4d30-a17b-66816a1a4a30
⛔ Files ignored due to path filters (3)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_device.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
android/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/utils/BootstrapStepExt.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/CoveApp.swiftios/Cove/CoverView.swiftios/CoveCore/Sources/CoveCore/CoveCore.swiftrust/Cargo.tomlrust/src/bootstrap.rsrust/src/bootstrap/migration.rsrust/src/database/migration.rsrust/src/database/migration/v1_bdk.rsrust/src/database/migration/v1_redb.rsrust/src/fiat/client.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/database/migration.rs
praveenperera
force-pushed
the
fix-splash-stall
branch
from
bc9f3ad to
58d3665
Compare
Contributor
Author
|
@greptile-apps re-review |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
ios/Cove/CoverView.swift (1)
86-102:⚠️ Potential issue | 🟠 MajorAdd the non-migration slow-start fallback message.
On Lines 87-100, status is only updated when migration progress exists, so slow non-migration boots still show no feedback. This was flagged earlier and is still present.
Proposed fix
.task { + let startTime = ContinuousClock.now while !Task.isCancelled { do { try await Task.sleep(for: .milliseconds(66)) } catch { break } if let progress = activeMigration()?.progress(), progress.total > 0 { statusMessage = "Encrypting data..." encryptionProgress = Double(progress.current) / Double(progress.total) - } else if encryptionProgress != nil { - statusMessage = nil - encryptionProgress = nil + } else { + encryptionProgress = nil + if ContinuousClock.now - startTime >= .seconds(10) { + statusMessage = "Taking longer than expected..." + } else { + statusMessage = nil + } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoverView.swift` around lines 86 - 102, The loop in the .task only updates statusMessage when activeMigration()?.progress() exists, so add a non-migration slow-start fallback: track elapsed time inside the task (e.g., record a start timestamp or tick counter in the loop) and if no migration progress is present after a short threshold (e.g., 400–600ms) set statusMessage to a brief fallback like "Starting…" (and leave encryptionProgress nil); continue to clear that fallback when a migration appears or when encryptionProgress is set to nil/when Task.isCancelled. Update the task's logic around activeMigration(), statusMessage and encryptionProgress to include this timed fallback and use a small boolean or timestamp property on the view to avoid repeatedly setting the message.android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt (1)
389-415:⚠️ Potential issue | 🟠 MajorStill missing slow-start feedback for non-migration boots.
On Lines 397-405 and 407-414, status updates are migration-only; slow startup without migration still never shows “Taking longer than expected...”.
Proposed fix
private suspend fun watchBootstrap( bootstrapDeferred: kotlinx.coroutines.Deferred<*>, onMigrationProgress: (status: String?, progress: Float?) -> Unit, ) { val startTime = System.currentTimeMillis() var migrationDetected = false var progressCleared = true + var slowStatusShown = false while (bootstrapDeferred.isActive) { delay(66) val step = bootstrapProgress() if (!migrationDetected && step.isMigrationInProgress) { migrationDetected = true } val progress = activeMigration()?.progress() if (progress != null && progress.total > 0u) { migrationDetected = true progressCleared = false onMigrationProgress("Encrypting data...", progress.current.toFloat() / progress.total.toFloat()) } else if (!progressCleared) { progressCleared = true onMigrationProgress(null, null) } val elapsed = System.currentTimeMillis() - startTime + if (!migrationDetected && elapsed >= 10_000L && !slowStatusShown) { + slowStatusShown = true + onMigrationProgress("Taking longer than expected...", null) + } // longer timeout to accommodate low-end Android hardware val timeoutMs = if (migrationDetected) 60_000L else 20_000L if (elapsed >= timeoutMs && bootstrapDeferred.isActive) { Log.w(TAG, "[STARTUP] watchdog firing after ${elapsed}ms (timeout=${timeoutMs}ms, migration=$migrationDetected)") cancelBootstrap() throw BootstrapTimeoutException() } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt` around lines 389 - 415, The loop only posts progress messages during migrations, so slow starts without migration never show the “Taking longer than expected...” message; update the watchdog logic inside the while (bootstrapDeferred.isActive) loop to emit a non-migration status before timing out: after computing elapsed and before firing the timeout branch, if migrationDetected is false and elapsed exceeds a shorter threshold (e.g., when timeoutMs reflects non-migration) call onMigrationProgress("Taking longer than expected...", null) (ensure progressCleared is set appropriately), otherwise preserve existing migration progress behavior; modify references around bootstrapProgress(), activeMigration(), migrationDetected, progressCleared, startTime, timeoutMs, and the timeout branch that calls cancelBootstrap()/throw BootstrapTimeoutException() so the UI shows slow-start feedback for non-migration boots.rust/src/database/migration/v1_redb.rs (1)
159-186:⚠️ Potential issue | 🔴 CriticalDo not classify redb verification I/O failures as corruption
Line 159-Line 186 returns
Ok(false)for operational failures (EncryptedBackend::open,create_with_backend,begin_read,list_tables). In recovery (Line 252-Line 261),Ok(false)drives restore-from-backup, so transient errors can incorrectly roll back a valid encrypted DB.Proposed fix
- let backend = match EncryptedBackend::open(path, key) { - Ok(b) => b, - Err(e) => { - warn!("Verification failed for {path_display}: could not open encrypted backend: {e}"); - return Ok(false); - } - }; + let backend = EncryptedBackend::open(path, key).context(format!( + "verification: could not open encrypted backend for {path_display}" + ))?; - let db = match redb::Database::builder().create_with_backend(backend) { - Ok(db) => db, - Err(e) => { - warn!("Verification failed for {path_display}: could not create database: {e}"); - return Ok(false); - } - }; + let db = redb::Database::builder() + .create_with_backend(backend) + .context(format!("verification: could not create database for {path_display}"))?; - let read_txn = match db.begin_read() { - Ok(txn) => txn, - Err(e) => { - warn!("Verification failed for {path_display}: could not begin read transaction: {e}"); - return Ok(false); - } - }; + let read_txn = db.begin_read().context(format!( + "verification: could not begin read transaction for {path_display}" + ))?; - if let Err(e) = read_txn.list_tables() { - warn!("Verification failed for {path_display}: could not list tables: {e}"); - return Ok(false); - } + read_txn + .list_tables() + .context(format!("verification: could not list tables for {path_display}"))?; Ok(true)As per coding guidelines, "Verify correct functionality and logic consistency." and "Suggest improvements to error handling, unwraps, and Result handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/migration/v1_redb.rs` around lines 159 - 186, The verification code currently maps operational I/O failures from EncryptedBackend::open, redb::Database::builder().create_with_backend, db.begin_read, and read_txn.list_tables to Ok(false) which is treated as corruption and triggers a restore; instead, propagate those operational errors as Err(...) (or a distinct error variant) so callers can distinguish transient I/O failures from genuine corruption, and only return Ok(false) for explicit corruption checks; update the match arms for those four calls to log the error but return Err(e.into() or a wrapped error) rather than Ok(false), and ensure the caller that handles the verification result treats Err as a retry/operational failure path rather than restore-from-backup.rust/src/database/migration/v1_bdk.rs (1)
28-60:⚠️ Potential issue | 🔴 CriticalPropagate operational verification failures as
Err, notOk(false)Line 28-Line 60 currently maps DB open/key/schema/integrity query errors to
Ok(false). In this flow,Ok(false)is treated as corruption and triggers backup restore in Line 273-Line 287, so transient I/O/locking failures can cause unintended rollback/data-loss behavior.Proposed fix
- let conn = match rusqlite::Connection::open(path) { - Ok(c) => c, - Err(e) => { - warn!("Verification failed for {path_display}: could not open connection: {e}"); - return Ok(false); - } - }; + let conn = rusqlite::Connection::open(path) + .context(format!("verification: could not open connection for {path_display}"))?; - if let Err(e) = conn.pragma_update(None, "key", &hex_key) { - warn!("Verification failed for {path_display}: pragma key failed: {e}"); - return Ok(false); - }; + conn.pragma_update(None, "key", &hex_key) + .context(format!("verification: pragma key failed for {path_display}"))?; - if let Err(e) = - conn.query_row("SELECT count(*) FROM sqlite_master WHERE type='table'", [], |r| { - r.get::<_, i64>(0) - }) - { - warn!("Verification failed for {path_display}: schema query failed: {e}"); - return Ok(false); - } + conn.query_row("SELECT count(*) FROM sqlite_master WHERE type='table'", [], |r| { + r.get::<_, i64>(0) + }) + .context(format!("verification: schema query failed for {path_display}"))?; match conn.query_row("PRAGMA integrity_check", [], |r| r.get::<_, String>(0)) { Ok(ref result) if result == "ok" => Ok(true), Ok(result) => { warn!("Verification failed for {path_display}: integrity check returned: {result}"); Ok(false) } - Err(e) => { - warn!("Verification failed for {path_display}: integrity check failed: {e}"); - Ok(false) - } + Err(e) => Err(e) + .context(format!("verification: integrity check failed for {path_display}")), }As per coding guidelines, "Verify correct functionality and logic consistency." and "Suggest improvements to error handling, unwraps, and Result handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/migration/v1_bdk.rs` around lines 28 - 60, The verification routine currently converts operational errors from rusqlite::Connection::open, conn.pragma_update, conn.query_row (schema) and the integrity_check Err branch into Ok(false), which wrongly signals corruption to the backup-restore logic; change these branches to propagate the underlying error as Err(...) instead of returning Ok(false) (e.g., return Err(e.into()) or map to the function's error type) while preserving the semantic that a successful integrity_check returning a non-"ok" string remains Ok(false); update the error-handling paths for rusqlite::Connection::open, conn.pragma_update, conn.query_row (schema) and the Err arm of the integrity_check match to return Err.
🧹 Nitpick comments (1)
rust/src/bootstrap.rs (1)
300-309: Prevent regressiveBOOTSTRAP_STEPwrites inset_step.Line 304 detects regressions, but Line 308 still assigns the lower/equal step. Returning early keeps progress monotonic in release builds too.
Proposed fix
fn set_step(step: BootstrapStep) { let mut current = BOOTSTRAP_STEP.lock(); if step <= *current { error!("bootstrap step regression: {current:?} -> {step:?}"); - } - - debug_assert!(step > *current, "bootstrap step must advance: {current:?} -> {step:?}"); + debug_assert!(false, "bootstrap step must advance: {current:?} -> {step:?}"); + return; + } *current = step; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/bootstrap.rs` around lines 300 - 309, The set_step function currently logs regressions but still writes a non-advancing value into BOOTSTRAP_STEP; change it so that when the new step is <= *current you log the error, keep the existing debug_assert!(step > *current, ...), and then return early (before assigning *current = step) to prevent regressive writes to BOOTSTRAP_STEP in release builds. Ensure this change is applied inside the set_step function and references BOOTSTRAP_STEP and the BootstrapStep parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt`:
- Around line 397-402: The watchdog loop currently calls activeMigration()
(returns an AutoCloseable Migration) and doesn't close it, leaking FFI handles;
wrap the activeMigration() call in a .use { migration -> ... } block (or check
for null then call .use) and move the progress() inspection and the existing
logic (setting migrationDetected/progressCleared and calling
onMigrationProgress) inside that block so the Migration is always closed after
use; ensure you preserve the else branch that checks !progressCleared
outside/after the use block as before.
---
Duplicate comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt`:
- Around line 389-415: The loop only posts progress messages during migrations,
so slow starts without migration never show the “Taking longer than expected...”
message; update the watchdog logic inside the while (bootstrapDeferred.isActive)
loop to emit a non-migration status before timing out: after computing elapsed
and before firing the timeout branch, if migrationDetected is false and elapsed
exceeds a shorter threshold (e.g., when timeoutMs reflects non-migration) call
onMigrationProgress("Taking longer than expected...", null) (ensure
progressCleared is set appropriately), otherwise preserve existing migration
progress behavior; modify references around bootstrapProgress(),
activeMigration(), migrationDetected, progressCleared, startTime, timeoutMs, and
the timeout branch that calls cancelBootstrap()/throw
BootstrapTimeoutException() so the UI shows slow-start feedback for
non-migration boots.
In `@ios/Cove/CoverView.swift`:
- Around line 86-102: The loop in the .task only updates statusMessage when
activeMigration()?.progress() exists, so add a non-migration slow-start
fallback: track elapsed time inside the task (e.g., record a start timestamp or
tick counter in the loop) and if no migration progress is present after a short
threshold (e.g., 400–600ms) set statusMessage to a brief fallback like
"Starting…" (and leave encryptionProgress nil); continue to clear that fallback
when a migration appears or when encryptionProgress is set to nil/when
Task.isCancelled. Update the task's logic around activeMigration(),
statusMessage and encryptionProgress to include this timed fallback and use a
small boolean or timestamp property on the view to avoid repeatedly setting the
message.
In `@rust/src/database/migration/v1_bdk.rs`:
- Around line 28-60: The verification routine currently converts operational
errors from rusqlite::Connection::open, conn.pragma_update, conn.query_row
(schema) and the integrity_check Err branch into Ok(false), which wrongly
signals corruption to the backup-restore logic; change these branches to
propagate the underlying error as Err(...) instead of returning Ok(false) (e.g.,
return Err(e.into()) or map to the function's error type) while preserving the
semantic that a successful integrity_check returning a non-"ok" string remains
Ok(false); update the error-handling paths for rusqlite::Connection::open,
conn.pragma_update, conn.query_row (schema) and the Err arm of the
integrity_check match to return Err.
In `@rust/src/database/migration/v1_redb.rs`:
- Around line 159-186: The verification code currently maps operational I/O
failures from EncryptedBackend::open,
redb::Database::builder().create_with_backend, db.begin_read, and
read_txn.list_tables to Ok(false) which is treated as corruption and triggers a
restore; instead, propagate those operational errors as Err(...) (or a distinct
error variant) so callers can distinguish transient I/O failures from genuine
corruption, and only return Ok(false) for explicit corruption checks; update the
match arms for those four calls to log the error but return Err(e.into() or a
wrapped error) rather than Ok(false), and ensure the caller that handles the
verification result treats Err as a retry/operational failure path rather than
restore-from-backup.
---
Nitpick comments:
In `@rust/src/bootstrap.rs`:
- Around line 300-309: The set_step function currently logs regressions but
still writes a non-advancing value into BOOTSTRAP_STEP; change it so that when
the new step is <= *current you log the error, keep the existing
debug_assert!(step > *current, ...), and then return early (before assigning
*current = step) to prevent regressive writes to BOOTSTRAP_STEP in release
builds. Ensure this change is applied inside the set_step function and
references BOOTSTRAP_STEP and the BootstrapStep parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c9a67f7-e774-432e-98b5-334caa41fff9
⛔ Files ignored due to path filters (3)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_device.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
android/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/utils/BootstrapStepExt.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/CoveApp.swiftios/Cove/CoverView.swiftios/CoveCore/Sources/CoveCore/CoveCore.swiftrust/Cargo.tomlrust/src/bootstrap.rsrust/src/bootstrap/migration.rsrust/src/database/migration.rsrust/src/database/migration/v1_bdk.rsrust/src/database/migration/v1_redb.rsrust/src/fiat/client.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- android/app/src/main/java/org/bitcoinppl/cove/utils/BootstrapStepExt.kt
- ios/CoveCore/Sources/CoveCore/CoveCore.swift
Comment on lines
+397
to
+402
| val progress = activeMigration()?.progress() | ||
| if (progress != null && progress.total > 0u) { | ||
| migrationDetected = true | ||
| progressCleared = false | ||
| onMigrationProgress("Encrypting data...", progress.current.toFloat() / progress.total.toFloat()) | ||
| } else if (!progressCleared) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Migration binding: class + close semantics =="
rg -n -C2 --type=kt '\bclass\s+Migration\b|\bAutoCloseable\b|\bfun\s+close\s*\(' android/app/src/main/java/org/bitcoinppl/cove_core
echo "== activeMigration signature =="
rg -n -C3 --type=kt '\bfun\s+activeMigration\s*\(' android/app/src/main/java/org/bitcoinppl/cove_core
echo "== activeMigration usage and cleanup patterns =="
rg -n -C3 --type=kt '\bactiveMigration\s*\(' android/app/src/main/java/org/bitcoinppl/cove
rg -n -C3 --type=kt '\b(activeMigration\s*\(\)\s*\.use|\.close\s*\()' android/app/src/main/java/org/bitcoinppl/coveRepository: bitcoinppl/cove
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Directory structure =="
ls -la android/app/src/main/java/org/bitcoinppl/cove_core/ 2>/dev/null | head -20
ls -la android/app/src/main/java/org/bitcoinppl/cove/ 2>/dev/null | head -20
echo "== Find kotlin files =="
fd --type f '\.kt$' android/app/src/main/java/org/bitcoinppl/cove
echo "== Search for Migration class =="
rg -n 'class\s+Migration' android/app/src/main/java/org/bitcoinppl/
echo "== Search for activeMigration function =="
rg -n 'fun\s+activeMigration' android/app/src/main/java/org/bitcoinppl/
echo "== Search for AutoCloseable in bindings =="
rg -n 'AutoCloseable' android/app/src/main/java/org/bitcoinppl/Repository: bitcoinppl/cove
Length of output: 13400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for activeMigration definition (broader) =="
rg -n 'activeMigration' android/app/src/main/java/org/bitcoinppl/ -A 2
echo "== Check MainActivity.kt watchdog loop context around 397-402 =="
sed -n '380,420p' android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt
echo "== Check for .progress() method on Migration =="
rg -n 'fun\s+progress' android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt | head -20
echo "== Check for existing .use patterns with activeMigration =="
rg -n -C2 'activeMigration.*\.use' android/app/src/main/java/org/bitcoinppl/coveRepository: bitcoinppl/cove
Length of output: 2894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Migration class definition and close method =="
sed -n '13697,13750p' android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt
echo "== Search for progress() method on Migration =="
rg -n 'fun progress' android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt | head -10
echo "== Check all activeMigration() usages in cove (not cove_core) =="
rg -n 'activeMigration' android/app/src/main/java/org/bitcoinppl/cove --type-add 'kt:*.kt'
echo "== Check for .use or .close patterns with Migration objects =="
rg -n '\.use.*Migration\|Migration.*\.close' android/app/src/main/java/org/bitcoinppl/cove --type-add 'kt:*.kt'
echo "== Search Migration class close method =="
sed -n '13697,13800p' android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt | grep -n 'close\|dispose'Repository: bitcoinppl/cove
Length of output: 1974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All usages of activeMigration in MainActivity =="
rg -n 'activeMigration' android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt
echo "== Check progress() return type and implementation =="
grep -n 'fun progress\|data class MigrationProgress' android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt | head -5
echo "== Get more context on progress() method =="
sed -n '13750,13850p' android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt | head -50
echo "== Check if MigrationProgress is AutoCloseable =="
rg -n 'data class MigrationProgress' android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt -A 10Repository: bitcoinppl/cove
Length of output: 2370
Close the Migration object returned by activeMigration() in the watchdog loop.
Line 397 fetches activeMigration() on every tick and it returns an AutoCloseable object. Not closing it can leak FFI handles over repeated loop iterations. Use .use { ... } to ensure cleanup.
Proposed fix
- val progress = activeMigration()?.progress()
+ val progress = activeMigration()?.use { it.progress() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt` around lines
397 - 402, The watchdog loop currently calls activeMigration() (returns an
AutoCloseable Migration) and doesn't close it, leaking FFI handles; wrap the
activeMigration() call in a .use { migration -> ... } block (or check for null
then call .use) and move the progress() inspection and the existing logic
(setting migrationDetected/progressCleared and calling onMigrationProgress)
inside that block so the Migration is always closed after use; ensure you
preserve the else branch that checks !progressCleared outside/after the use
block as before.
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
1 participant
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.
Remove initData() from the critical startup path — it only spawns background price/fee fetches and is not needed for first render. Fire it in a detached Task instead so app/auth are set immediately after bootstrap, preventing the splash from getting stuck if the FFI async boundary stalls.
Add slow-startup feedback ("Taking longer than expected...") to the splash screen after 10 seconds. Add granular info!() logging to Rust bootstrap steps for diagnostics.
Summary by CodeRabbit