feat: add crash log persistence and its ui tab - #271
Conversation
Installs a Thread.UncaughtExceptionHandler that captures the last logcat lines, custom log source entries, and recent network requests alongside the exception's stack trace, then always delegates to whatever handler was previously installed so other crash reporters keep working. Records are written synchronously to the no-backup directory and surfaced in a new Crash tab on the next launch, with copy/share/delete support.
Crash persistence is now always-on with a fixed internal line cap, matching the issue's ask instead of exposing configurability nobody requested.
CrashRecordStorage and BugReportDraftStorage each had an identical canonical-path safety check before delete, just under different names.
A failed request previously rendered identically to a successful one in the crash text export apart from a null status code. Appending the error title/message (not the full stack trace) keeps each line scannable while surfacing the one detail most likely to matter for crash triage.
DebugOverlay.install() runs on the main thread before Application.onCreate() via AndroidX Startup, so the previous synchronous getAppInfo() call (multiple PackageManager IPC calls) directly delayed cold start. It's now fetched in a background coroutine and read via an AtomicReference-backed provider lambda; a crash before that completes just gets a null appInfo, same as any other best-effort field in this codebase.
…andler The platform always installs a default UncaughtExceptionHandler before any app code runs, so previousHandler is never actually null here. Rather than reimplementing the platform's own kill-process fallback for a branch that can't be reached (and can't be safely unit-tested as written), just no-op: the crash record is already persisted by the time this runs.
Down to one line after dropping the no-previous-handler fallback; no need for a separate function.
writeSync() (called from uncaughtException()) previously listed, sorted, and deleted old records on every write. Retention doesn't need real-time enforcement, so eviction now happens in listCrashRecords() instead - already off the crash path, on Dispatchers.IO, only when the Crash tab is opened. writeSync() is now just "build filename, write file."
crashRecords was a one-shot flow cached via stateIn(Lazily), so deleting a record removed the file on disk but left the stale list in the UI until the app restarted. Switched to the same lazy-load-once + explicit-refresh pattern BugReportDraftStorage already uses for drafts, and re-run it after every delete.
|
Claude finished @Manabu-GT's task in 4m 47s —— View job Code review: crash log persistence + Crash tab
Overall this is a well-built feature. The crash path is carefully designed — chaining rather than replacing the previous handler, Five things I'd address before merge, four of them small. Details are in the inline comments. Findings
On #1 — this is the one genuine correctness bug. On #2 — AndroidX Startup runs before Smaller notes
SecurityNothing blocking. Records land in ValidationReview is based on reading |
Manabu-GT
changed the title
📝 WalkthroughWalkthroughThe change adds synchronous crash capture, durable crash-record storage, crash-log export, and a crash-log tab in the debug panel. It also centralizes FileProvider URI creation and strengthens shared file path validation. ChangesCrash record lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DebugOverlay
participant CrashHandler
participant DebugOverlayDataRepository
participant CrashRecordStorage
participant DebugPanelDialog
DebugOverlay->>CrashHandler: install uncaught-exception handler
CrashHandler->>DebugOverlayDataRepository: writeCrashRecordSync(thread, throwable)
DebugOverlayDataRepository->>CrashRecordStorage: persist crash record
DebugPanelDialog->>DebugOverlayDataRepository: observe crash records
DebugPanelDialog->>CrashRecordStorage: delete selected record
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Qodana for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at qodana-support@jetbrains.com
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9070e7ee6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@AGENTS.md`:
- Line 34: Update the “Modify crash persistence” entry in AGENTS.md to use
repository-relative file paths with line numbers for every referenced Kotlin
file and the DebugOverlayDataRepository.writeCrashRecordSync() location,
following the required path/to/File.kt:42 format.
In
`@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashHandler.kt`:
- Around line 33-37: Update CrashHandler installation or its uncaughtException
fallback so a null previousHandler never results in a no-op after crash capture;
preserve platform uncaught-exception behavior by avoiding installation when no
handler exists or delegating to a safe equivalent fallback. Add a regression
test covering previousHandler = null and verifying the fallback behavior.
In
`@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordStorage.kt`:
- Around line 58-61: Validate maxRecords in DefaultCrashRecordStorage before it
reaches the eviction logic, rejecting negative values or normalizing them to
zero so files.sortedDescending().drop(maxRecords) cannot throw. Add coverage for
negative maxRecords configuration, preserving normal eviction behavior for
non-negative values.
In
`@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/DebugOverlayDataRepository.kt`:
- Around line 173-177: Serialize the delete-and-refresh sequence in
deleteCrashRecord by protecting both crashRecordStorage.deleteCrashRecord(info)
and refreshCrashRecords() with a shared Mutex.withLock block, ensuring
concurrent deletions cannot overwrite newer snapshots. Add a test that invokes
concurrent deletions and verifies the Crash tab records remain correctly empty.
In
`@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/CrashLogDetailScreen.kt`:
- Around line 82-86: Update the share action in CrashLogDetailScreen’s
IconButton coroutine to inspect the result of CrashRecordExporter.share and show
an error message when it reports failure before the coroutine completes. Reuse
the screen’s existing user-feedback mechanism and preserve the current behavior
for successful sharing.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c80af46c-aa8e-47f9-8844-523e289f0fe4
📒 Files selected for processing (29)
AGENTS.mddebugoverlay-core/src/main/AndroidManifest.xmldebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/DebugOverlay.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/bugreport/BugReportDraftStorage.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/bugreport/IntentShareExporter.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/bugreport/model/BugReportSnapshot.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/bugreport/ui/BugReporterFab.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashHandler.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecord.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordBuilder.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordExporter.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordStorage.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordTextFormatter.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/DebugOverlayDataRepository.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/CountBadge.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/CrashLogDetailScreen.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/CrashLogTabContent.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/DebugPanelDialog.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/util/FileProviders.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/util/Files.ktdebugoverlay-core/src/main/res/values/strings.xmldebugoverlay-core/src/main/res/xml/debugoverlay_bugreport_paths.xmldebugoverlay-core/src/main/res/xml/debugoverlay_file_provider_paths.xmldebugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/internal/crash/CrashHandlerTest.ktdebugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordBuilderTest.ktdebugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordStorageTest.ktdebugoverlay-core/src/test/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordTextFormatterTest.ktdocs/ARCHITECTURE.md
💤 Files with no reviewable changes (1)
- debugoverlay-core/src/main/res/xml/debugoverlay_bugreport_paths.xml
| internal class DefaultCrashRecordStorage( | ||
| private val context: Context, | ||
| private val maxRecords: Int = DEFAULT_MAX_CRASH_RECORDS, | ||
| ) : CrashRecordStorage { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate maxRecords before eviction.
If maxRecords is negative, files.sortedDescending().drop(maxRecords) in debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordStorage.kt:106 throws IllegalArgumentException when records exist. Reject negative values in the constructor, or normalize them to zero. Add coverage for this configuration.
🤖 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
`@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordStorage.kt`
around lines 58 - 61, Validate maxRecords in DefaultCrashRecordStorage before it
reaches the eviction logic, rejecting negative values or normalizing them to
zero so files.sortedDescending().drop(maxRecords) cannot throw. Add coverage for
negative maxRecords configuration, preserving normal eviction behavior for
non-negative values.
| fun deleteCrashRecord(info: CrashRecordInfo) { | ||
| scope.launch { | ||
| crashRecordStorage.deleteCrashRecord(info) | ||
| refreshCrashRecords() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'DebugOverlayDataRepository\.kt|build\.gradle|gradle\.properties|kotlin' || true
echo "== target file outline =="
ast-grep outline debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/DebugOverlayDataRepository.kt --view expanded || true
echo "== target file relevant lines =="
cat -n debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/DebugOverlayDataRepository.kt | sed -n '1,260p'
echo "== kotlin/coroutines version references =="
rg -n "kotlinx-coroutines|kotlin_version|kotlin|coroutines" -S build.gradle.kts settings.gradle.kts gradle.properties debugoverlay-core/build.gradle.kts . 2>/dev/null | head -200 || trueRepository: Manabu-GT/DebugOverlay-Android
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
repo_path = Path('debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/DebugOverlayDataRepository.kt')
text = repo_path.read_text()
ops = []
for name in ['refreshCrashRecords', 'deleteCrashRecord']:
m = re.search(f'(^\\s*private\\s+suspend\\s+fun\\s+{name}\\(|^\\s*fun\\s+{name}\\()', text, flags=re.M)
if not m:
print(f"{name}: not found")
continue
start = m.start()
depth = 0
i = m.end() - 1
found = False
while i < len(text):
if text[i] == '{':
depth += 1
found = True
elif text[i] == '}':
depth -= 1
if found and depth == 0:
ops.append((name, text[m.start():i+1]))
break
i += 1
for name, body in ops:
print(f"== {name} ==")
print(body)
print("contains Mutex:", "Mutex" in body)
print("contains withLock:", "withLock" in body)
print("repo imports Mutex:", "import kotlinx.coroutines.sync.Mutex" in text)
print("repo imports withLock:", "import kotlinx.coroutines.sync.*" in text or "kotlinx.coroutines.sync" in text)
PYRepository: Manabu-GT/DebugOverlay-Android
Length of output: 826
🌐 Web query:
kotlinx.coroutines 1.10.2 core API Mutex withLock kotlinx.coroutines.sync.Mutex
💡 Result:
In kotlinx.coroutines, the withLock extension function is the recommended way to use a Mutex for protecting critical sections in coroutine-based code [1][2]. It is defined as a suspending inline function within the kotlinx.coroutines.sync package [3][4]. Key characteristics and usage: Safety and Pattern: The withLock function executes a given action while holding the mutex lock [5][4]. It is safer than manual lock/unlock calls because it uses a try-finally block to ensure that the lock is always released after the action completes, regardless of whether the action succeeds or throws an exception [1]. Non-Reentrant: Unlike the JVM synchronized block, Mutex (and by extension withLock) is non-reentrant [4][6]. If a coroutine that already holds the lock attempts to acquire it again, it will suspend, which can lead to deadlocks if not handled carefully [4][6]. Owner Token: The withLock function accepts an optional owner parameter [5][4]. If an owner is provided (a non-null value) and the mutex is already locked by the same owner, an IllegalStateException will be thrown [5][1]. This is primarily a debugging aid [4][1]. Usage Example: val mutex = Mutex suspend fun criticalSection { mutex.withLock { // Protected code here } } Because withLock is a suspending function, it must be called from within a coroutine or another suspending function [6]. It provides memory consistency guarantees similar to synchronized blocks on the JVM: an unlock operation happens-before every subsequent successful lock operation on that same Mutex [4].
Citations:
- 1: https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-core/common/src/sync/Mutex.kt
- 2: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.sync/-mutex/lock.html
- 3: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.sync/
- 4: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.sync/-mutex/
- 5: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.sync/with-lock.html
- 6: https://discuss.kotlinlang.org/t/is-it-always-safe-to-just-convert-synchronized-to-mutex-withlock/26519
Serialize deletion and refresh operations.
Concurrent deleteCrashRecord() calls launch independent coroutines, and refreshCrashRecords() writes _crashRecords.value with a fresh listCrashRecords() snapshot. If two deletes run concurrently, an older snapshot can overwrite a newer empty snapshot and make the Crash tab show a record after it is deleted. Wrap delete-plus-refresh in one Mutex.withLock block and add a concurrent-deletion test.
🤖 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
`@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/DebugOverlayDataRepository.kt`
around lines 173 - 177, Serialize the delete-and-refresh sequence in
deleteCrashRecord by protecting both crashRecordStorage.deleteCrashRecord(info)
and refreshCrashRecords() with a shared Mutex.withLock block, ensuring
concurrent deletions cannot overwrite newer snapshots. Add a test that invokes
concurrent deletions and verifies the Crash tab records remain correctly empty.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordExporter.kt (1)
27-33: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftPropagate share failures to the caller.
runCatchingNonCancellation { ... }.onFailure { ... }produces aResult, butshareis declared to returnUnit.withContextreturns its block result, and Kotlin coerces aUnit-expected block toUnit, so thisResultis discarded. AwriteText,debugOverlayFileUri, orstartActivityfailure is logged and appears successful to the caller. ReturnResult<Unit>or rethrow after logging, then let the UI report the failure. (kotlinlang.org)After the change, run
./gradlew spotlessApplyand then./gradlew check.Also applies to: 52-54
🤖 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 `@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordExporter.kt` around lines 27 - 33, Update CrashRecordExporter.share so failures from runCatchingNonCancellation, including writeText, debugOverlayFileUri, or startActivity, are propagated to the caller instead of being discarded by the Unit return type. Return Result<Unit> or rethrow after logging, preserving cancellation behavior and allowing the UI to report failures; then run spotlessApply and check.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordExporter.kt`:
- Around line 27-33: Update CrashRecordExporter.share so failures from
runCatchingNonCancellation, including writeText, debugOverlayFileUri, or
startActivity, are propagated to the caller instead of being discarded by the
Unit return type. Return Result<Unit> or rethrow after logging, preserving
cancellation behavior and allowing the UI to report failures; then run
spotlessApply and check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e95b168b-260d-4a51-ac0b-07232a4640f0
📒 Files selected for processing (6)
debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/DebugOverlay.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecord.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordExporter.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordStorage.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/DebugOverlayDataRepository.ktdebugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecord.kt
- debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/DebugOverlay.kt
- debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/crash/CrashRecordStorage.kt
- debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/source/LogcatDataSource.kt
- debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/DebugOverlayDataRepository.kt
* docs: update README for v2.7.0 - Bump dependency coordinate examples in README from 2.6.3 to 2.7.0. - Add the Crash tab to the Debug Panel list and a short "Crash logs" usage section covering the new crash log persistence (#231). - Drop the "Logcat buffer size" paragraph about `logcat -T N` / `-t N`: the one-shot capture is gone, the reader no longer starts on panel open, and the replayed history is no longer tied to maxLogcatEntries. - Add CHANGELOG entry for v2.7.0 covering crash log persistence (#271), the switch to streaming logcat from app start, and AGP 9.3.1 (#241). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 participants
Summary by CodeRabbit
New Features
Bug Fixes