Improve de-/encrypting db error handling #698
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR enhances biometric lock security by adding database backup and integrity verification during enable/disable operations. It introduces exception handling with user-facing error dialogs, converts key biometric functions to suspend functions, and updates string resources and localization for bug reporting functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Layer<br/>(Activity/Fragment)
participant Scope as lifecycleScope
participant Model as BaseNoteModel
participant IO as IO Context
participant DB as Database
participant Encrypt as Encryption Utils
rect rgb(200, 220, 255)
Note over UI,Encrypt: Biometric Enable Flow
UI->>Scope: launch coroutine
Scope->>Model: enableBiometricLock(cipher)
Model->>IO: withContext(IO)
IO->>DB: copyDatabase (backup)
IO->>Encrypt: encryptDatabase(backup)
IO->>DB: overwrite original with encrypted
IO->>DB: validateEncrypted() check
alt Validation Success
IO->>Model: write lock state + key
Model-->>Scope: return success
Scope->>UI: update UI + show toast
else Validation Fails
IO->>Model: throw EncryptionException
Model-->>Scope: propagate exception
Scope->>UI: catch + showErrorDialog
end
end
rect rgb(200, 255, 220)
Note over UI,Encrypt: Biometric Disable Flow
UI->>Scope: launch coroutine
Scope->>Model: disableBiometricLock()
Model->>IO: withContext(IO)
IO->>DB: copyDatabase(decrypt=true)
IO->>Encrypt: decryptDatabase(copy)
IO->>DB: overwrite original with decrypted
IO->>DB: validateUnencrypted() check
alt Validation Success
IO->>Model: disable lock + callback
Model-->>Scope: return success
Scope->>UI: show disable toast
else Validation Fails
IO->>Model: throw DecryptionException
Model-->>Scope: propagate exception
Scope->>UI: catch + showErrorDialog
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
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/LockedActivity.kt (1)
87-114: Catch failures fromdisableBiometricLock()before showing success toast.
disableBiometricLock()now performs IO and throwsEncryptionException/DecryptionExceptionwhen crypto fails. Launching it without atry/catchonlifecycleScopehands any failure to the default coroutine exception handler, crashing the activity and leaving the UI hidden. Wrap both calls in atry/catch(orrunCatching) and route errors through the newshowErrorDialoghelper instead of unconditionally showing the success toast.
🧹 Nitpick comments (2)
app/src/main/java/com/philkes/notallyx/utils/security/DecryptionException.kt (1)
1-3: Remove the empty class body.Detekt flags the trailing
{}as an empty class block. Drop the braces so the declaration is a single line.-class DecryptionException(msg: String, cause: Throwable? = null) : Exception(msg, cause) {} +class DecryptionException(msg: String, cause: Throwable? = null) : Exception(msg, cause)app/src/main/java/com/philkes/notallyx/utils/security/EncryptionException.kt (1)
1-3: Remove the empty class body.For consistency with detekt, make the declaration a single line without
{}.-class EncryptionException(msg: String, cause: Throwable? = null) : Exception(msg, cause) {} +class EncryptionException(msg: String, cause: Throwable? = null) : Exception(msg, cause)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (23)
TRANSLATIONS.md(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/activity/LockedActivity.kt(4 hunks)app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt(6 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt(4 hunks)app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt(2 hunks)app/src/main/java/com/philkes/notallyx/utils/ErrorActivity.kt(2 hunks)app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt(1 hunks)app/src/main/java/com/philkes/notallyx/utils/security/DecryptionException.kt(1 hunks)app/src/main/java/com/philkes/notallyx/utils/security/EncryptionException.kt(1 hunks)app/src/main/java/com/philkes/notallyx/utils/security/EncryptionUtils.kt(1 hunks)app/src/main/res/values-cs/strings.xml(1 hunks)app/src/main/res/values-de/strings.xml(1 hunks)app/src/main/res/values-es/strings.xml(1 hunks)app/src/main/res/values-fr/strings.xml(1 hunks)app/src/main/res/values-it/strings.xml(1 hunks)app/src/main/res/values-nl/strings.xml(1 hunks)app/src/main/res/values-pl/strings.xml(1 hunks)app/src/main/res/values-ro/strings.xml(1 hunks)app/src/main/res/values-ru/strings.xml(1 hunks)app/src/main/res/values-zh-rCN/strings.xml(1 hunks)app/src/main/res/values-zh-rTW/strings.xml(1 hunks)app/src/main/res/values/strings.xml(2 hunks)documentation/docs/faq.md(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
app/src/main/java/com/philkes/notallyx/utils/ErrorActivity.kt (1)
app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt (2)
showErrorDialog(186-208)showErrorDialog(210-232)
app/src/main/java/com/philkes/notallyx/presentation/activity/LockedActivity.kt (2)
app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt (3)
showToast(918-918)showToast(920-923)showToast(925-928)app/src/main/java/com/philkes/notallyx/presentation/view/note/action/ActionBottomSheet.kt (1)
hide(139-141)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
app/src/main/java/com/philkes/notallyx/utils/security/EncryptionUtils.kt (3)
encryptDatabase(20-33)decryptDatabase(35-48)decryptDatabase(56-66)
app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt (1)
app/src/main/java/com/philkes/notallyx/utils/security/EncryptionUtils.kt (3)
getInitializedCipherForDecryption(112-120)decryptDatabase(35-48)decryptDatabase(56-66)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt (2)
app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt (3)
showToast(918-918)showToast(920-923)showToast(925-928)app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt (2)
showErrorDialog(186-208)showErrorDialog(210-232)
🪛 detekt (1.23.8)
app/src/main/java/com/philkes/notallyx/utils/security/DecryptionException.kt
[warning] 3-3: The class or object DecryptionException is empty.
(detekt.empty-blocks.EmptyClassBlock)
app/src/main/java/com/philkes/notallyx/utils/security/EncryptionException.kt
[warning] 3-3: The class or object EncryptionException is empty.
(detekt.empty-blocks.EmptyClassBlock)
🔇 Additional comments (15)
app/src/main/res/values-es/strings.xml (1)
258-258: ✓ Localization update looks good.The string update shortens the "report_bug" text from "Informar de incidente/error" to "Informar error" for consistency with the English "Report Bug" update. This is a straightforward localization improvement with no functional impact.
app/src/main/res/values-zh-rCN/strings.xml (1)
264-264: ✓ Localization update looks good.The string update simplifies the Chinese (Simplified) translation from "报告问题/错误" to "报告错误", maintaining clarity while improving consistency across the localization suite.
app/src/main/res/values-pl/strings.xml (1)
269-269: ✓ Localization update looks good.The Polish translation is shortened from "Zgłoś problem/błąd" to "Zgłoś błąd", maintaining meaning while aligning with the global UI text update.
app/src/main/res/values-it/strings.xml (1)
249-249: ✓ Localization update looks good.The Italian translation is updated from "Segnala un problema/bug" to "Segnala bug", providing a more concise and consistent phrasing.
app/src/main/res/values-zh-rTW/strings.xml (1)
251-251: ✓ Localization update looks good.The Chinese (Traditional) translation is simplified from "報告問題/錯誤" to "報告錯誤", maintaining semantic clarity while improving brevity.
app/src/main/res/values-nl/strings.xml (1)
187-187: ✓ Localization update looks good.The Dutch translation is updated from "Rapporteer een probleem/fout" to "Bug melden", providing a cleaner, more direct phrasing consistent with the global update.
app/src/main/res/values-ru/strings.xml (1)
264-264: ✓ Localization update looks good.The Russian translation is trimmed from "Сообщить об ошибке/баге" to "Сообщить об ошибке", removing redundancy while maintaining clarity.
app/src/main/res/values-fr/strings.xml (1)
258-258: ✓ Localization update looks good.The French translation is shortened from "Signaler un problème/un bug" to "Signaler bug", providing more concise UI text while maintaining the intended meaning.
app/src/main/res/values-cs/strings.xml (1)
264-264: LGTM!Translation update is consistent and properly formatted.
app/src/main/res/values-de/strings.xml (1)
265-265: LGTM!Translation update is consistent and properly formatted.
app/src/main/res/values-ro/strings.xml (1)
258-258: LGTM!Translation update is consistent and properly formatted.
app/src/main/res/values/strings.xml (2)
49-51: Well‑structured error strings with actionable messaging.The new strings provide clear error feedback with proper format placeholders for inserting the report action label. String naming convention is clear and consistent.
Please verify these new error strings are used in the encryption/decryption failure handling code paths mentioned in the PR description.
273-273: LGTM!Label simplification is clear and consistent with locale-specific updates.
TRANSLATIONS.md (1)
22-51: Verify translation coverage reflects new strings across all locales.The table shows total increased from 312 to 315 strings. However, spot-checking the Czech row: if previously at 301/312 (~96%), adding 3 strings should yield approximately 304/315 (~96%), but it shows 301/315 (~95%). This suggests the new biometric error strings may not yet be translated in all locales.
Confirm whether this is intentional (new strings marked as untranslated) or if translations are still pending.
documentation/docs/faq.md (1)
97-97: LGTM!Documentation wording updates are consistent with the simplified "Report Bug" label across the app. User instructions remain clear and actionable.
Also applies to: 146-146
...n/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt
Show resolved
Hide resolved
| showErrorDialog( | ||
| throwable, | ||
| auto_backup_failed, | ||
| getString(crash_export_backup_failed, this.getString(report_bug)), | ||
| originalStacktrace = stacktrace, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Import showErrorDialog to fix the unresolved reference.
showErrorDialog(...) lives in com.philkes.notallyx.presentation. Without the import this file won’t compile.
import com.philkes.notallyx.presentation.setupProgressDialog
import com.philkes.notallyx.presentation.showToast
+import com.philkes.notallyx.presentation.showErrorDialogCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/com/philkes/notallyx/utils/ErrorActivity.kt around lines 97
to 103, the call to showErrorDialog is an unresolved reference because the
function is defined in com.philkes.notallyx.presentation; add the missing import
by adding import com.philkes.notallyx.presentation.showErrorDialog near the
other imports at the top of the file so the call compiles, then re-run build to
confirm resolution.
app/src/main/java/com/philkes/notallyx/utils/security/EncryptionUtils.kt
Show resolved
Hide resolved
Crustack
force-pushed
the
fix/decrypt-encrypt-errors
branch
from
f569244 to
d0e8ac2
Compare
With this PR dis-/enabling biometric lock becomes much safer
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation