Refactor: extract `CrashReportService` interface to `:common` by criticalAY · Pull Request #20739 · ankidroid/Anki-Android

criticalAY · 2026-04-13T19:12:15Z

Note

Assisted-by Claude Opus 4.6
Updating the imports (not all but most)

Purpose / Description

Move crash report to common module so we can avoid circular dependency

Fixes

Fixes part of Architecture Discussion: Module Extraction #20737

Approach

See commits

How Has This Been Tested?

Build

Learning (optional, can help others)

NA

Checklist

Please, go through these checks before submitting the PR.

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

david-allison

Looks great!

I don't think the exception classes need to be moved:

CombinedException is only used in 1 feature
InvalidSearchException should be above libanki, not below it
ManuallyReportedException should be rewritten
StorageAccessException should be above libanki, not below it
UnknownDatabaseVersionException should be above libanki, not below it
UserSubmittedException - used in 1 feature

criticalAY · 2026-04-14T07:35:43Z

I will rebase later, but I have addressed the concerns

david-allison

Looks good!

david-allison

Code LGTM!

Nits, as this is a significant architectural pattern/change

This one needs some more thought on the commits: you apply a change then revert it in the next commit.

Also see: https://github.com/ankidroid/Anki-Android/blob/main/AI_POLICY.md#disclosure

The main commit message is especially important, because the change introduces an architectural pattern into the codebase. Could you humanize it a bit, to explain the 'why'.

criticalAY · 2026-04-14T17:05:28Z

@david-allison fyi

Note

Assisted-by Claude Opus 4.6
Updating the imports (not all but most)

Re review

david-allison

This is structured unusually which makes it hard to follow:

In commit 1, there are 2 objects called CrashReportService

I would recommend:

commit 0: any pre-fixes which could be split out of commit 1, for example any changes to the tests/adding the init method
commit 1: introduce the interface and make the implementation private
commit 2: move to the correct package

david-allison · 2026-04-15T09:30:36Z

+    @VisibleForTesting
+    fun getReporter(): CrashReporter = instance
+
+    fun sendExceptionReport(


These are the methods which are called publicly, but there's no documentation on them

(Maybe you could get this for free by implementing the interface)

david-allison

LGTM, assuming CI is happy

Extract CrashReportService.initialize() and isProperServiceProcess() into top-level functions.

As we move towards a multi-module architecture, feature modules need crash reporting without depending on the app module. Without this, each feature module would need its own bridge interface (e.g. WidgetCrashReporter, BrowserCrashReporter) duplicating the same pattern everywhere. This does use android dependencies though and those don't seem removable, so it gets the android-capable sub-module of common

Move CrashReportService from com.ichi2.anki to com.ichi2.anki.common.crashreporting and update imports. Assisted-by: Claude Opus 4 (1M context)

mikehardy

seems fine to me - I just de-conflicted after the merge in of common:android PR since this needs android functionality and wouldn't compile otherwise

(just a move of file directly from common/src/main/etc to common/android/sra/main/etc

#20731

Also de-conflicted imports change collision in ProfileManager since that merged today as well

Trying to keep everything moving while I have a minute :)

github-actions bot added the Needs Review label Apr 13, 2026

criticalAY removed the Needs Review label Apr 13, 2026

criticalAY marked this pull request as draft April 13, 2026 19:14

criticalAY force-pushed the refactor/crashreport-ext branch from c3210c0 to d983c00 Compare April 13, 2026 19:29

criticalAY changed the title ~~Refactor: CrashReporter in :common module~~ Refactor: extract CrashReportService interface to :common Apr 13, 2026

criticalAY marked this pull request as ready for review April 13, 2026 19:50

criticalAY added the Needs Review label Apr 13, 2026

criticalAY force-pushed the refactor/crashreport-ext branch from d983c00 to 38871b1 Compare April 14, 2026 05:24

david-allison reviewed Apr 14, 2026

View reviewed changes

Comment thread AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ACRATest.kt Outdated

david-allison requested changes Apr 14, 2026

View reviewed changes

david-allison added the Needs Author Reply Waiting for a reply from the original author label Apr 14, 2026

criticalAY force-pushed the refactor/crashreport-ext branch from 38871b1 to e8acb11 Compare April 14, 2026 07:34

criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Apr 14, 2026

david-allison reviewed Apr 14, 2026

View reviewed changes

Comment thread common/src/main/java/com/ichi2/anki/common/crash/CrashReportService.kt Outdated

Comment thread common/src/main/java/com/ichi2/anki/common/crash/CrashReportService.kt Outdated

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/dialogs/BackupPromptDialog.kt Outdated

david-allison mentioned this pull request Apr 14, 2026

Architecture Discussion: Module Extraction #20737

Open

4 tasks

criticalAY force-pushed the refactor/crashreport-ext branch 2 times, most recently from cb830b0 to 558698a Compare April 14, 2026 09:49

david-allison previously requested changes Apr 14, 2026

View reviewed changes

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/AcraCrashReporter.kt Outdated

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/AcraCrashReporter.kt

Comment thread common/android/src/main/java/com/ichi2/anki/common/crashreporting/CrashReportService.kt

criticalAY force-pushed the refactor/crashreport-ext branch from 558698a to 9b607ae Compare April 14, 2026 17:02

david-allison self-requested a review April 15, 2026 06:46

david-allison requested changes Apr 15, 2026

View reviewed changes

david-allison added the Needs Author Reply Waiting for a reply from the original author label Apr 15, 2026

david-allison mentioned this pull request Apr 15, 2026

feat: analytics interface in :common #20736

Open

5 tasks

github-actions bot added the Has Conflicts label Apr 15, 2026

criticalAY force-pushed the refactor/crashreport-ext branch 2 times, most recently from bf1d13f to da10a61 Compare April 17, 2026 19:07

criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Apr 17, 2026

criticalAY removed the Has Conflicts label Apr 17, 2026

david-allison approved these changes Apr 17, 2026

View reviewed changes

david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Apr 17, 2026

github-actions bot added the Has Conflicts label Apr 19, 2026

criticalAY force-pushed the refactor/crashreport-ext branch from da10a61 to 580ce1a Compare April 19, 2026 15:23

criticalAY removed the Has Conflicts label Apr 19, 2026

github-actions bot added the Has Conflicts label Apr 19, 2026

criticalAY added 3 commits April 19, 2026 17:11

refactor: add initializeAcraCrashReporter() top-level function

b9712a8

Extract CrashReportService.initialize() and isProperServiceProcess() into top-level functions.

refactor: move CrashReportService to crashreporting package

a38a657

Move CrashReportService from com.ichi2.anki to com.ichi2.anki.common.crashreporting and update imports. Assisted-by: Claude Opus 4 (1M context)

mikehardy force-pushed the refactor/crashreport-ext branch from 580ce1a to a38a657 Compare April 19, 2026 22:35

mikehardy approved these changes Apr 19, 2026

View reviewed changes

mikehardy enabled auto-merge April 19, 2026 22:47

mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge Has Conflicts labels Apr 19, 2026

mikehardy added this pull request to the merge queue Apr 19, 2026

Merged via the queue into ankidroid:main with commit d529ff1 Apr 19, 2026
15 checks passed

github-actions bot added this to the 2.24 release milestone Apr 19, 2026

github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Apr 19, 2026

Uh oh!

Conversation

criticalAY commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Learning (optional, can help others)

Checklist

Uh oh!

Uh oh!

david-allison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

criticalAY commented Apr 14, 2026

Uh oh!

david-allison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-allison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

criticalAY commented Apr 14, 2026

Uh oh!

david-allison left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-allison Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-allison left a comment

Choose a reason for hiding this comment

Uh oh!

mikehardy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

criticalAY commented Apr 13, 2026 •

edited

Loading

david-allison left a comment •

edited

Loading

david-allison Apr 15, 2026 •

edited

Loading

mikehardy left a comment •

edited

Loading