Ignore corrupted notes on load + offer cleanup + raw database import/export#1045
Open
Crustack wants to merge 8 commits into
Open
Ignore corrupted notes on load + offer cleanup + raw database import/export#1045Crustack wants to merge 8 commits into
Crustack wants to merge 8 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUnifies importer progress types to Progress; adds raw-database SAF export/import and readBaseNotes; centralizes converter error reporting (ConverterErrorReporter) and makes converters return safe defaults on failure; tracks corrupted import rows and threads corruption counts through import flows; updates UI, dialogs, and DAOs for import/export and cleanup. Raw DB import/export & resilience
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (2)
945-951: 💤 Low valueVerify that
cleanupDatabaseachieves intended corruption cleanup.The method reads all notes and immediately writes them back via
updateAll. The cleanup behavior appears to rely on implicit side effects during the write operation (e.g., converter error handling that resets corrupted fields). Consider adding a clarifying comment or renaming toreprocessAllNotesto make the intent clearer.🤖 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 `@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt` around lines 945 - 951, The current cleanupDatabase function simply reads all notes via baseNoteDao.getAllSync() and writes them back with baseNoteDao.updateAll(allNotes), relying on implicit side effects to fix corrupted fields; either rename the function to reprocessAllNotes to reflect that it reserializes/deserializes every record, or keep the name and add a precise comment above cleanupDatabase explaining that the intent is to trigger converter/DAO-side repair by re-saving all entities (mentioning baseNoteDao.getAllSync and baseNoteDao.updateAll), so callers understand it does not perform explicit validation but forces reprocessing.
413-424: ⚡ Quick winConsider more specific error message for raw database imports.
The error toast displays
R.string.invalid_backup, but when importing a raw database, "invalid database" might be clearer than "invalid backup" since the user is selecting a.sqlitefile, not a backup archive.💬 Suggested message refinement
Consider adding a dedicated string resource for raw database import errors, or using a more generic message:
val exceptionHandler = CoroutineExceptionHandler { _, throwable -> app.log(TAG, throwable = throwable) - app.showToast("${app.getString(R.string.invalid_backup)}: ${throwable.message}") + app.showToast("${app.getString(R.string.error)}: ${throwable.message}") }🤖 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 `@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt` around lines 413 - 424, The exception handler in importRawDatabase uses R.string.invalid_backup which is misleading for raw .sqlite imports; update the handler to use a more specific message (e.g., create and use R.string.invalid_database or a generic R.string.invalid_file) and call app.showToast with that string (via app.getString(...)) so users see "invalid database" for importRawDatabase failures; update any references in the CoroutineExceptionHandler block (importRawDatabase, app.showToast, R.string.invalid_backup) accordingly.app/src/main/java/com/philkes/notallyx/utils/ErrorActivity.kt (1)
199-221: 🏗️ Heavy liftConsider restoring the safety copy on reimport failure.
The reimport flow creates a database copy (
_BEFORE_REIMPORT) at line 204 but never restores it if the reimport fails. Users experiencing reimport failures are left with a deleted database and must manually recover.🔄 Suggested safety improvement
Restore the safety copy in the reimport exception handler:
lifecycleScope.launch(exceptionHandler) { val importResult = withContext(Dispatchers.IO) { val (_, databaseCopy) = copyDatabase(suffix = "_BEFORE_REIMPORT") try { deleteDatabase(NotallyDatabase.DATABASE_NAME) NotallyDatabase.clearInstance() application.importRawDatabase(uri, exportBackupProgress) } catch (e: Exception) { // Restore safety copy before propagating error databaseCopy.copyToLarge( NotallyDatabase.getCurrentDatabaseFile(application), overwrite = true ) throw e } } // ... show success dialog }🤖 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 `@app/src/main/java/com/philkes/notallyx/utils/ErrorActivity.kt` around lines 199 - 221, The reimport routine creates a safety copy via copyDatabase(suffix = "_BEFORE_REIMPORT") but never restores it on failure; modify the withContext(Dispatchers.IO) block so that after creating val (_, databaseCopy) you wrap the deleteDatabase(NotallyDatabase.DATABASE_NAME)/NotallyDatabase.clearInstance()/application.importRawDatabase(uri, exportBackupProgress) sequence in a try/catch, and in the catch restore the safety copy by calling databaseCopy.copyToLarge(NotallyDatabase.getCurrentDatabaseFile(application), overwrite = true) (or equivalent restore method) before rethrowing the exception; this ensures the original DB is restored if importRawDatabase fails.app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt (1)
138-138: 💤 Low valueRemove or clarify outdated TODO comment.
The comment suggests error handling is missing, but the
toListfunction (lines 447-451) already wraps each row conversion in try-catch and counts failures. Either remove this TODO or clarify what additional handling is needed.🤖 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 `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt` at line 138, The TODO comment "// TODO: add error handling if single note fails" is outdated because the toList function already wraps each row conversion in try-catch and counts failures; either remove that TODO or replace it with a clarifying comment that references the existing toList behavior (e.g., "per-note failures are caught and counted in toList") so future readers know where per-item errors are handled; update the comment near ImportExtensions.kt (the earlier TODO) to either be deleted or changed to explicitly point to the toList implementation and any additional desired behavior if more handling is required.
🤖 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 `@app/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.kt`:
- Around line 132-135: Two DAO methods duplicate the same query and return type:
getAll() and getAllSync() in BaseNoteDao; consolidate to one method (pick a
single name, e.g., getAll()) and remove the other, then update all call sites
(e.g., change usages in CleanupMissingAttachmentsWorker.kt and BaseNoteModel.kt
to the chosen method name). Make sure the DAO method signature and return type
remain List<BaseNote> and adjust imports or references if necessary.
In `@app/src/main/java/com/philkes/notallyx/data/dao/CommonDao.kt`:
- Line 103: The code initializes a local mutable counter corrupted from
readCorrupted and increments corrupted when insertions fail, but the function
returns readCorrupted instead, discarding the increments; update the
return/result assembly to use the mutated corrupted variable (not readCorrupted)
so corruptedNotes count reflects skipped/failed inserts. Locate the usages in
CommonDao.kt where corrupted is declared/updated (the corrupted/readCorrupted
pair) and change the returned value(s) at the end of those import/processing
functions (also repeat the same fix in the second similar block around the other
occurrence) to return the local corrupted variable.
In `@app/src/main/java/com/philkes/notallyx/data/imports/NotesImporter.kt`:
- Around line 109-113: The duplicate count is currently computed as
totalCandidates - insertedCount which incorrectly includes corrupted notes;
change the computation where ImportResult(...) is returned (in
NotesImporter/ImportResult creation) to subtract corruptedCount as well, i.e.
set duplicates = totalCandidates - insertedCount - corruptedCount so duplicates
reflect only actual duplicate notes (use the existing insertedCount,
corruptedCount and totalCandidates symbols).
In `@app/src/main/java/com/philkes/notallyx/data/NotallyDatabase.kt`:
- Around line 117-119: clearInstance() must remove the forever LiveData
observers and close the old RoomDatabase before nulling the singleton: obtain
the current instance.value (the RoomDatabase created by createInstance), call
preferences.biometricLock.removeObserver(biometricLockObserver) and
preferences.dataInPublicFolder.removeObserver(dataInPublicFolderObserver)
(matching the observers registered via
preferences.biometricLock.observeForeverSkipFirst /
preferences.dataInPublicFolder.observeForeverSkipFirst), then call
instance.value.close() to release DB resources (so
importRawDatabase/ErrorActivity won’t leak file handles), and only after
removing observers and closing the DB set instance = null; ensure you reference
the existing fields biometricLockObserver and dataInPublicFolderObserver and
perform null-safe checks around instance/value and the observers.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt`:
- Around line 567-597: The selection index is offset by two display items, so
assigned selectedImportSource = ImportSource.entries[which - 1] is wrong; change
the assignment to use which - 2 (i.e., selectedImportSource =
ImportSource.entries[which - 2]) and add a guard to ensure which >= 2 before
indexing to avoid out-of-bounds; update the code around the when(...) block
where selectedImportSource is set (referencing selectedImportSource and
ImportSource.entries) to apply this fix.
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt`:
- Around line 321-328: In ContextWrapper.exportRawDatabase, handle the case
where contentResolver.openOutputStream(fileUri) returns null and report errors:
check the nullable result before use, log or surface an error (via
Timber/processLogger/throw an exception or return a failure result) if the
OutputStream is null so the caller/user is notified, and also catch IOExceptions
from FileInputStream(databaseCopy) and inputStream.copyToLarge to log/report the
specific failure; update callers of exportRawDatabase or the function signature
if needed to propagate the error instead of silently succeeding.
- Around line 329-335: Replace the fragile assert-based size check with explicit
validation: inside the contentResolver.query(...) use block (the cursor handling
that reads OpenableColumns.SIZE) verify cursor.moveToFirst() and read sizeIndex;
if the size is missing or <= 0 then throw or return a descriptive exception
(e.g., IOException or IllegalStateException) instead of assert so it works in
production. Also ensure the temporary file referenced as databaseCopy is cleaned
up: wrap the code that creates/uses databaseCopy in a try/finally (or use
Kotlin’s use-block equivalent) and call databaseCopy.delete() (or
deleteRecursively() if a directory) in the finally to guarantee deletion even on
error. Include these changes around the contentResolver.query block and any code
that constructs/uses databaseCopy.
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt`:
- Around line 95-110: The importRawDatabase function currently leaks the
InputStream and leaves the temporary tempDbFile behind; fix by opening the
InputStream with Kotlin's use (e.g.,
contentResolver.openInputStream(dbFileUri).use { stream -> ... }) so the stream
is always closed, and ensure tempDbFile.delete() is called in a finally block
(or use try { ... } finally { tempDbFile.delete() }) after readBaseNotes/import
so the temp file is removed regardless of success or exception; apply these
changes around the calls in importRawDatabase (symbols: importRawDatabase,
stream, tempDbFile, readBaseNotes, import, importProgress).
- Around line 119-146: The opened SQLiteDatabase in ContextWrapper.readBaseNotes
(variable database) is never closed; wrap the database usage in a safe close
pattern (e.g., database.use { ... } or try { ... } finally { database.close() })
so the DB is always closed even on exceptions, and also ensure labelCursor and
baseNoteCursor are closed (use cursor.use { ... } or close them in finally)
while keeping the existing logic that builds BaseNotesImport (baseNotes,
originalIds, labels, corrupted).
In `@app/src/main/java/com/philkes/notallyx/utils/ErrorActivity.kt`:
- Line 196: The TODO should be resolved by using the reimport failure's
stacktrace rather than the original crash stacktrace: inside ErrorActivity's
reimport error handler (the code path that catches the reimport Throwable, e.g.,
in the method handling reimport failures), capture the reimport Throwable's
stacktrace (for example via throwable.stackTraceToString() or
Log.getStackTraceString(reimportThrowable)) and pass that string (or the
reimport Throwable itself if the receiver accepts a Throwable) instead of the
prior "stacktrace" variable so the logged/returned stacktrace reflects the
reimport error.
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 945-951: The current cleanupDatabase function simply reads all
notes via baseNoteDao.getAllSync() and writes them back with
baseNoteDao.updateAll(allNotes), relying on implicit side effects to fix
corrupted fields; either rename the function to reprocessAllNotes to reflect
that it reserializes/deserializes every record, or keep the name and add a
precise comment above cleanupDatabase explaining that the intent is to trigger
converter/DAO-side repair by re-saving all entities (mentioning
baseNoteDao.getAllSync and baseNoteDao.updateAll), so callers understand it does
not perform explicit validation but forces reprocessing.
- Around line 413-424: The exception handler in importRawDatabase uses
R.string.invalid_backup which is misleading for raw .sqlite imports; update the
handler to use a more specific message (e.g., create and use
R.string.invalid_database or a generic R.string.invalid_file) and call
app.showToast with that string (via app.getString(...)) so users see "invalid
database" for importRawDatabase failures; update any references in the
CoroutineExceptionHandler block (importRawDatabase, app.showToast,
R.string.invalid_backup) accordingly.
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt`:
- Line 138: The TODO comment "// TODO: add error handling if single note fails"
is outdated because the toList function already wraps each row conversion in
try-catch and counts failures; either remove that TODO or replace it with a
clarifying comment that references the existing toList behavior (e.g., "per-note
failures are caught and counted in toList") so future readers know where
per-item errors are handled; update the comment near ImportExtensions.kt (the
earlier TODO) to either be deleted or changed to explicitly point to the toList
implementation and any additional desired behavior if more handling is required.
In `@app/src/main/java/com/philkes/notallyx/utils/ErrorActivity.kt`:
- Around line 199-221: The reimport routine creates a safety copy via
copyDatabase(suffix = "_BEFORE_REIMPORT") but never restores it on failure;
modify the withContext(Dispatchers.IO) block so that after creating val (_,
databaseCopy) you wrap the
deleteDatabase(NotallyDatabase.DATABASE_NAME)/NotallyDatabase.clearInstance()/application.importRawDatabase(uri,
exportBackupProgress) sequence in a try/catch, and in the catch restore the
safety copy by calling
databaseCopy.copyToLarge(NotallyDatabase.getCurrentDatabaseFile(application),
overwrite = true) (or equivalent restore method) before rethrowing the
exception; this ensures the original DB is restored if importRawDatabase fails.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd547302-30d1-49e1-a75e-0eacad4edb97
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (21)
app/src/main/java/com/philkes/notallyx/data/NotallyDatabase.ktapp/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.ktapp/src/main/java/com/philkes/notallyx/data/dao/CommonDao.ktapp/src/main/java/com/philkes/notallyx/data/imports/ExternalImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/NotesImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/evernote/EvernoteImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/google/GoogleKeepImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/json/JsonImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/quillpad/QuillpadImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/txt/PlainTextImporter.ktapp/src/main/java/com/philkes/notallyx/data/model/ConverterErrorReporter.ktapp/src/main/java/com/philkes/notallyx/data/model/Converters.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/ErrorActivity.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.ktapp/src/main/res/drawable/database.xmlapp/src/main/res/layout/dialog_error.xmlapp/src/main/res/values/strings.xml
💤 Files with no reviewable changes (1)
- app/src/main/res/layout/dialog_error.xml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt (1)
268-272:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEnsure
EditNoteActivityimport exists inMainActivity.kt(Lines 268-272)
setupFAB()referencesEditNoteActivity::class.java, butEditNoteActivityis declared incom.philkes.notallyx.presentation.activity.note, soMainActivity.ktmust importcom.philkes.notallyx.presentation.activity.note.EditNoteActivity.🐛 Restore the import
import com.philkes.notallyx.presentation.activity.note.EditListActivity +import com.philkes.notallyx.presentation.activity.note.EditNoteActivityCollecting
ConverterErrorReporter.errorsvia a plainlifecycleScope.launch { collect { ... } }is not lifecycle-state aware; preferrepeatOnLifecycle(STARTED)to avoid showing dialogs when the activity isn’t in a started state.🤖 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 `@app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt` around lines 268 - 272, MainActivity.kt is missing an import for EditNoteActivity used in setupFAB: add import com.philkes.notallyx.presentation.activity.note.EditNoteActivity so setupFAB can reference EditNoteActivity::class.java and continue calling prepareNewNoteIntent(intent); additionally replace the plain lifecycleScope.launch { ConverterErrorReporter.errors.collect { ... } } collection with a lifecycle-aware pattern using lifecycleScope.launch { repeatOnLifecycle(Lifecycle.State.STARTED) { ConverterErrorReporter.errors.collect { ... } } } so error dialogs are only shown while the activity is in STARTED state (reference symbols: setupFAB, prepareNewNoteIntent, EditNoteActivity, ConverterErrorReporter.errors, lifecycleScope.launch, repeatOnLifecycle(STARTED)).
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt (1)
101-118: ⚡ Quick winCollect lifecycle-aware to avoid showing dialogs while the activity is stopped.
lifecycleScope.launch { errors.collect { ... } }keeps collecting from STARTED through STOPPED (it is only cancelled atonDestroy). If a converter error arrives while the activity is in the background,showErrorDialogwill attempt to show a dialog on a non-resumed activity, risking a window leak /IllegalStateExceptionafter state has been saved. Wrap the collection inrepeatOnLifecycle(Lifecycle.State.STARTED)so it only runs while visible.♻️ Lifecycle-aware collection
- lifecycleScope.launch { - ConverterErrorReporter.errors.collect { throwable -> - ... - } - } + lifecycleScope.launch { + repeatOnLifecycle(Lifecycle.State.STARTED) { + ConverterErrorReporter.errors.collect { throwable -> + ... + } + } + }Add imports:
+import androidx.lifecycle.Lifecycle +import androidx.lifecycle.repeatOnLifecycleKotlin androidx lifecycle repeatOnLifecycle recommended pattern for collecting flows in an Activity🤖 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 `@app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt` around lines 101 - 118, The current lifecycleScope.launch collecting ConverterErrorReporter.errors (lifecycleScope.launch { ConverterErrorReporter.errors.collect { ... } }) can emit while the Activity is stopped and cause dialogs to be shown on a non-resumed Activity; change this to use lifecycle.repeatOnLifecycle(Lifecycle.State.STARTED) so collection only runs while the Activity is visible—wrap the collection block in lifecycleScope.launch { repeatOnLifecycle(Lifecycle.State.STARTED) { ConverterErrorReporter.errors.collect { ... } } }, keep the existing handling (application.log, showErrorDialog, ConverterErrorReporter.registerDialog, baseModel.cleanupDatabase) unchanged, and add the necessary import for repeatOnLifecycle/Lifecycle.
🤖 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
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt`:
- Around line 101-118: Multiple emitted converter errors currently spawn a
dialog per emission; coalesce them by changing the collector on
ConverterErrorReporter.errors to either use flow operators (e.g.,
ConverterErrorReporter.errors.conflate() or .debounce(300L)) or gate showing
with a local flag/activeDialog check so only one dialog is shown at a time;
implement by wrapping the existing lifecycleScope.launch collector (the block
that calls showErrorDialog, ConverterErrorReporter.registerDialog and
baseModel.cleanupDatabase) so before calling showErrorDialog you return early if
a dialog is already active, set the flag when you register the dialog and clear
it when dismissing (or hook into ConverterErrorReporter.dismissAllDialogs) to
ensure only a single cleanup prompt appears.
---
Outside diff comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt`:
- Around line 268-272: MainActivity.kt is missing an import for EditNoteActivity
used in setupFAB: add import
com.philkes.notallyx.presentation.activity.note.EditNoteActivity so setupFAB can
reference EditNoteActivity::class.java and continue calling
prepareNewNoteIntent(intent); additionally replace the plain
lifecycleScope.launch { ConverterErrorReporter.errors.collect { ... } }
collection with a lifecycle-aware pattern using lifecycleScope.launch {
repeatOnLifecycle(Lifecycle.State.STARTED) {
ConverterErrorReporter.errors.collect { ... } } } so error dialogs are only
shown while the activity is in STARTED state (reference symbols: setupFAB,
prepareNewNoteIntent, EditNoteActivity, ConverterErrorReporter.errors,
lifecycleScope.launch, repeatOnLifecycle(STARTED)).
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt`:
- Around line 101-118: The current lifecycleScope.launch collecting
ConverterErrorReporter.errors (lifecycleScope.launch {
ConverterErrorReporter.errors.collect { ... } }) can emit while the Activity is
stopped and cause dialogs to be shown on a non-resumed Activity; change this to
use lifecycle.repeatOnLifecycle(Lifecycle.State.STARTED) so collection only runs
while the Activity is visible—wrap the collection block in lifecycleScope.launch
{ repeatOnLifecycle(Lifecycle.State.STARTED) {
ConverterErrorReporter.errors.collect { ... } } }, keep the existing handling
(application.log, showErrorDialog, ConverterErrorReporter.registerDialog,
baseModel.cleanupDatabase) unchanged, and add the necessary import for
repeatOnLifecycle/Lifecycle.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbd41fd2-fa79-46ab-b615-b37c56ec3a2b
📒 Files selected for processing (2)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt
Contributor
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 `@app/src/main/java/com/philkes/notallyx/data/model/ConverterErrorReporter.kt`:
- Line 11: Make the mutable set activeDialogs private to prevent external
mutation: rename/replace the public mutable property activeDialogs with a
private mutableSet (e.g., _activeDialogs) and expose a read-only view via a
public val activeDialogs: Set<Dialog> get() = _activeDialogs.toSet(); update any
usages in ConverterErrorReporter (including registerDialog/unregisterDialog or
other methods that mutate the set) to reference _activeDialogs so external
callers can only read the snapshot but must use the class' register/unregister
APIs to change membership.
- Line 11: Replace the non-thread-safe mutableSetOf used by the activeDialogs
field with a thread-safe collection to prevent races from registerDialog,
dismissAllDialogs and dismiss listeners; change the activeDialogs declaration to
a synchronized set (e.g. Collections.synchronizedSet(...)) and add the import
java.util.Collections, and also make activeDialogs private to improve
encapsulation so only the class methods (registerDialog, dismissAllDialogs,
etc.) mutate it.
- Line 9: Change the mutable reference to the AtomicBoolean so it cannot be
reassigned: replace the declaration "var enabled = AtomicBoolean(true)" with an
immutable property (val enabled = AtomicBoolean(true)) in ConverterErrorReporter
(so callers mutate state via enabled.set(...) instead of reassigning the
AtomicBoolean reference).
- Around line 25-29: dismissAllDialogs() currently iterates activeDialogs while
each dialog's dismiss listener may remove entries, causing
ConcurrentModificationException; fix by iterating over a snapshot (e.g.,
activeDialogs.toList()) when calling dismiss(), and if activeDialogs is a
synchronized set guard the snapshot creation plus activeDialogs.clear() and
errors.postValue(null) in the same synchronized block to ensure consistency
between dismissal and clearing; references: dismissAllDialogs(), activeDialogs,
errors, and the dialog dismiss listener that mutates activeDialogs.
- Line 14: The check reading LiveData.errors.value from a background thread in
ConverterErrorReporter (the if using enabled.get() && errors.value == null
inside reportError) is unsafe; replace that value read with a thread-safe flag
such as an AtomicReference<Throwable?> or AtomicBoolean that tracks whether an
error is already set, update that atomic from reportError (before calling
errors.postValue) and clear it in dismissAllDialogs, and keep using
errors.postValue for the LiveData update so no direct LiveData.value reads occur
off the main thread.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa0de967-3593-465f-be60-5ab241b18eea
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (5)
app/src/main/java/com/philkes/notallyx/data/model/ConverterErrorReporter.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.ktapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt
- app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt
- app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt
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.
Relates to #1029
Summary by CodeRabbit
New Features
Bug Fixes
Improvements