Bug reporting and diagnostic sending#832
Open
praveenperera wants to merge 30 commits into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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
bug-reporting
branch
from
df1391e to
0b3b6e5
Compare
Greptile SummaryThis PR adds diagnostic reporting and persistent diagnostic history. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "Correct coin control trace label" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
praveenperera
force-pushed
the
bug-reporting
branch
from
deb11fb to
ae6eb33
Compare
Add persistent Rust log capture and a diagnostics report API that assembles, redacts, gzips, and uploads reviewed reports. Keep release debug logging intentional while demoting noisy hot-path logs to trace.
Add Settings -> About entry points and short-lived iOS and Android review sheets for diagnostics. The flow lets users preview, share locally, clear stored logs, submit explicitly, and copy the returned report ID.
Regenerate the Swift and Kotlin Cove Core bindings for the new diagnostics report API.
Add description-based preview rendering and redaction for upload data, while updating Android and iOS diagnostics UI to hide/send-submit behavior in decoy mode and block dismissal while submitting. Harden response handling to limit and truncate diagnostic upload server responses.
Update log capture snapshots to read from rolling log files when file-backed logging is attached, instead of only using in-memory ring buffer text. Adds tests to cover restart persistence and archive ordering (oldest archive through current log) in snapshot output.
Split diagnostics sheet state, content, and platform helpers into dedicated files while preserving behavior.
Drop the helper subtitle under Send Diagnostics in the About screen to match the Android row presentation.
Attach persistent Rust log capture before storage bootstrap work so failed launches keep diagnostic logs across restarts. Also redact transaction IDs embedded in larger log tokens to keep previews and uploads aligned with the privacy notice.
Capture Swift log messages to rotated app-owned files so iOS diagnostics include app logs even though system logs are sandboxed. Keep Rust file logging off the tracing hot path with a writer thread while flushing before snapshots and clears.
Show user-oriented diagnostics upload errors, retry transient collector failures, and preserve multiline Swift log messages so diagnostics redaction sees their original shape.
Store recent diagnostics report IDs after successful uploads so the apps can recover them after the success alert is dismissed. Cap the history and expose read and clear operations through UniFFI.
Add settings entry points and platform-native history screens so users can revisit, copy, or clear recently submitted diagnostics report IDs.
Return a structured diagnostics submission so clients can surface a saved report ID warning when local history persistence fails. Validate collector success responses, keep decode failures distinct from request encoding, and avoid retaining plaintext redaction secrets in debug output.
Treat history load errors as an explicit failed state in Android and iOS instead of successful empty history. Keep the About entry visible when history is unavailable and add retry paths plus focused Android helper coverage.
Track Rust and Swift diagnostics log write errors so snapshots include a visible breadcrumb instead of silently dropping entries. Fall back to in-memory Rust logs when disk capture is incomplete and clear attached log files during local data reset.
Make SwiftLogStore.clear return errors when file operations fail and update callers and tests to propagate and validate failure cases.
Ensure reattaching diagnostics capture releases the current writer before opening a new one so shutdown callbacks can safely replace current log files.
Add git branch and build profile to diagnostics metadata. Use the captured values in preview generation and upload payload assertions to keep both surfaces consistent.
Use a no-redirect reqwest client and retry only connection failures during upload, because post and status errors may already have reached the collector.
Expand seed-phrase redaction to detect formatted and numbered mnemonic lists, while preserving valid BIP-39 constraints and preserving boundary formatting in output.
When users choose to clear stored diagnostics logs, the Android flow now also attempts to clear platform logcat output (`logcat -c`) before collecting new logs. The clear step is best-effort, runs on the IO dispatcher, and enforces the existing timeout with process cleanup to avoid hanging.
- Narrow broad `Exception` catches to specific types (`DiagnosticsException`, `IOException`, `ActivityNotFoundException`, etc.) in `SendDiagnosticsSheet` and `SubmittedDiagnosticsScreen` - Extract inline composables (`SubmittedDiagnosticsLoading`, `SubmittedDiagnosticsLoadError`, `SubmittedDiagnosticsErrorDialog`) to reduce nesting - Change `loadSubmittedDiagnosticsRecords` to accept a `Result`-returning lambda instead of a throwing one - Extract `new_test_wallet_actor` helper in Rust actor tests to reduce boilerplate - Add missing `global_state_test_lock` guards to two actor tests - Use `Mnemonic::parse_in_normalized` with explicit language in bip39 tests
praveenperera
force-pushed
the
bug-reporting
branch
from
68fb0a7 to
14ef915
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
ios/Cove/Flows/SettingsFlow/AboutScreen.swift (1)
49-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold summary/visibility logic onto the shared load-state type.
shouldShowSubmittedDiagnosticsduplicates the exact switch inSubmittedDiagnosticsLoadState.canClear, andsubmittedDiagnosticsSummaryre-implements the count/label logic that also exists on the Android side. Consider exposing these as computed properties onSubmittedDiagnosticsLoadStateto keep the platforms in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Cove/Flows/SettingsFlow/AboutScreen.swift` around lines 49 - 70, Move the submitted diagnostics visibility and summary logic from AboutScreen’s shouldShowSubmittedDiagnostics and submittedDiagnosticsSummary into computed properties on the shared SubmittedDiagnosticsLoadState type, including canClear-aligned visibility and count-based labeling; update AboutScreen to consume those properties and preserve existing behavior.rust/src/diagnostics/upload.rs (1)
10-10: 🔒 Security & Privacy | 🔵 TrivialConfirm the shipped
APP_TOKENis a non-secret client identifier with collector-side abuse protection.
APP_TOKENis compiled into the mobile binary and sent asbearer_auth, so it is extractable by anyone and cannot be treated as a per-user secret. That's acceptable for a shared app token, but the diagnostics collector should not rely on it for authentication/authorization and should enforce rate limiting / size limits / abuse protection independently. Please confirm this matches the collector's threat model, and prefer build-time injection over a source-committed constant if rotation is expected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/src/diagnostics/upload.rs` at line 10, The hardcoded APP_TOKEN in upload diagnostics must be treated only as a publicly extractable client identifier, not a secret or authorization credential. Verify the collector independently enforces authentication/authorization boundaries, rate limiting, payload-size limits, and abuse protection; if token rotation is expected, replace the source constant with build-time configuration while preserving a safe fallback or validation.rust/crates/cove-common/src/logging/capture.rs (1)
151-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeparate the guard from event formatting.
Add a blank line after the multi-line
let ... elseguard before formatting the event. As per coding guidelines, “Add blank lines between logical steps … after multi-lineif/match/guardblocks before the next independent statement.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/cove-common/src/logging/capture.rs` around lines 151 - 156, Add a blank line after the multi-line ReentrancyGuard `let ... else` block in the event-capture function, before the `format_event(event)` statement, keeping the guard and event formatting as separate logical steps.Source: Coding guidelines
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SubmittedDiagnosticsScreen.kt (1)
79-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKey
LaunchedEffectto an actual dependency instead ofUnit.Per the path instructions for this codebase,
LaunchedEffectshould be keyed to actual dependencies, notUnit. Keying toioDispatchersatisfies the rule without changing behavior — the effect will still run once on initial composition and re-run only if the dispatcher changes (which it won't in practice).♻️ Suggested change
- LaunchedEffect(Unit) { + LaunchedEffect(ioDispatcher) { loadState = loadSubmittedDiagnosticsRecords(ioDispatcher) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SubmittedDiagnosticsScreen.kt` around lines 79 - 81, Change the LaunchedEffect in SubmittedDiagnosticsScreen from Unit to the actual ioDispatcher dependency, so it runs on initial composition and only re-runs if the dispatcher changes. Keep the existing loadSubmittedDiagnosticsRecords call and state assignment unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SendDiagnosticsSheet.kt`:
- Around line 346-361: Clear the stale action error at the start of
SettingsFlow's share() method, before calling shareDiagnosticsFile, by resetting
actionError to its neutral state; preserve the existing exception handling so
new share failures still populate the error.
- Around line 86-88: Change the LaunchedEffect in SendDiagnosticsSheet to use
context as its key instead of Unit, while keeping the existing
state.rebuildReport(context, clearStoredLogs = false) call unchanged.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SubmittedDiagnosticsScreen.kt`:
- Around line 113-125: Wrap each Database() instance in
SubmittedDiagnosticsScreen, including the instance used in the coroutine’s
diagnosticsReports().clear() call, with use { } so it is closed automatically;
update both call sites consistently while preserving the existing exception
handling and behavior.
In `@ios/Cove/SwiftLogStore.swift`:
- Around line 118-121: Update the file creation logic in SwiftLogStore to apply
the .protectionKey file protection attribute when creating the diagnostics log
via fileManager.createFile, ensuring the unredacted log uses the required
explicit protection class.
In `@rust/crates/cove-common/src/logging/capture.rs`:
- Around line 91-100: Bound the writer queue used by LogWriterHandle and the
writer loop: replace the unbounded mpsc::channel with a bounded channel, make
Write submissions non-blocking, and drop or degrade writes when the queue is
full while retaining Flush and ClearAndWrite control commands. Update the
relevant send/receive handling around the writer task and preserve existing
ring-buffer and control-command behavior.
In `@rust/src/manager/send_flow_manager/psbt_builder.rs`:
- Line 77: Update the trace label in build_psbt_for_coin_control from
build_psbt_for_utxo_list to build_psbt_for_coin_control so diagnostics identify
the correct code path.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SubmittedDiagnosticsScreen.kt`:
- Around line 79-81: Change the LaunchedEffect in SubmittedDiagnosticsScreen
from Unit to the actual ioDispatcher dependency, so it runs on initial
composition and only re-runs if the dispatcher changes. Keep the existing
loadSubmittedDiagnosticsRecords call and state assignment unchanged.
In `@ios/Cove/Flows/SettingsFlow/AboutScreen.swift`:
- Around line 49-70: Move the submitted diagnostics visibility and summary logic
from AboutScreen’s shouldShowSubmittedDiagnostics and
submittedDiagnosticsSummary into computed properties on the shared
SubmittedDiagnosticsLoadState type, including canClear-aligned visibility and
count-based labeling; update AboutScreen to consume those properties and
preserve existing behavior.
In `@rust/crates/cove-common/src/logging/capture.rs`:
- Around line 151-156: Add a blank line after the multi-line ReentrancyGuard
`let ... else` block in the event-capture function, before the
`format_event(event)` statement, keeping the guard and event formatting as
separate logical steps.
In `@rust/src/diagnostics/upload.rs`:
- Line 10: The hardcoded APP_TOKEN in upload diagnostics must be treated only as
a publicly extractable client identifier, not a secret or authorization
credential. Verify the collector independently enforces
authentication/authorization boundaries, rate limiting, payload-size limits, and
abuse protection; if token rotation is expected, replace the source constant
with build-time configuration while preserving a safe fallback or validation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c16dead7-d2b5-4029-b6ba-e308d4e91371
⛔ Files ignored due to path filters (3)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktis excluded by!android/app/src/main/java/org/bitcoinppl/cove_core/**ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**,!ios/CoveCore/Sources/CoveCore/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/AboutSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/LogcatProcessCollector.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SendDiagnosticsContent.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SendDiagnosticsPlatform.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SendDiagnosticsSheet.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/SubmittedDiagnosticsScreen.ktandroid/app/src/test/java/org/bitcoinppl/cove/flows/SettingsFlow/LogcatProcessCollectorTest.ktandroid/app/src/test/java/org/bitcoinppl/cove/flows/SettingsFlow/SendDiagnosticsSheetHelpersTest.ktios/Cove.xcodeproj/project.pbxprojios/Cove/Flows/SettingsFlow/AboutScreen.swiftios/Cove/Flows/SettingsFlow/BackupExportView.swiftios/Cove/Flows/SettingsFlow/SendDiagnosticsSheet.swiftios/Cove/Flows/SettingsFlow/SubmittedDiagnosticsSheet.swiftios/Cove/SwiftLogStore.swiftios/CoveTests/SwiftLogStoreTests.swiftios/Logger.swiftrust/Cargo.tomlrust/crates/cove-bip39/src/lib.rsrust/crates/cove-common/Cargo.tomlrust/crates/cove-common/src/logging.rsrust/crates/cove-common/src/logging/capture.rsrust/crates/cove-http/src/lib.rsrust/crates/cove-tokio/src/debounced_task.rsrust/src/app.rsrust/src/bootstrap.rsrust/src/bootstrap/diagnostics.rsrust/src/database.rsrust/src/database/diagnostics_reports.rsrust/src/database/error.rsrust/src/diagnostics.rsrust/src/diagnostics/redact.rsrust/src/diagnostics/upload.rsrust/src/discovery_scanner.rsrust/src/lib.rsrust/src/manager/deferred_sender.rsrust/src/manager/send_flow_manager.rsrust/src/manager/send_flow_manager/amount_input.rsrust/src/manager/send_flow_manager/fee_selection.rsrust/src/manager/send_flow_manager/psbt_builder.rsrust/src/manager/wallet_manager/actor.rsrust/src/transaction_watcher.rs
💤 Files with no reviewable changes (1)
- rust/src/app.rs
Treat contiguous hex sequences of txid length or longer as a single transaction ID when the exact 64-character boundaries are unclear. This avoids leaking trailing hex data from larger runs, and adds a regression test for the prefixed txid case.
Bound Rust writer memory and surface dropped disk entries through the ring-buffer fallback. Apply explicit data protection to Swift logs.
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.
Summary by CodeRabbit
New Features
Bug Fixes