Fix/data migrations#840
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a guarded global uncaught-exception handler in LockedActivity to detect SQLiteBlobTooBigException, launch an asynchronous split/truncate migration (splitOversizedNotes) with progress reporting and mutex/timestamp deduplication; moves ViewModel observation into startObserving(); makes migration repair/delete resilient; truncates bodies during import; adds TimeMillis helper and secondsBetween. Changes
Sequence DiagramsequenceDiagram
participant LockedActivity as LockedActivity (onCreate)
participant BaseModel as BaseNoteModel
participant DB as Room/Database
participant ExceptionHandler as GlobalExceptionHandler
participant Migration as splitOversizedNotes
LockedActivity->>LockedActivity: initViewModel()
LockedActivity->>BaseModel: startObserving()
BaseModel->>DB: attach LiveData observers / run queries
DB-->>BaseModel: query throws SQLiteBlobTooBigException
DB->>ExceptionHandler: uncaught exception propagates
ExceptionHandler->>ExceptionHandler: check Mutex + timestamp
ExceptionHandler->>LockedActivity: create MigrationProgress LiveData
ExceptionHandler->>Migration: launch splitOversizedNotes() on IO
Migration->>DB: read, truncate/repair notes (or delete if unrecoverable)
Migration-->>LockedActivity: post progress updates
Migration->>DB: observers re-run and succeed
ExceptionHandler->>ExceptionHandler: release Mutex / update last timestamp
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 6
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/viewmodel/BaseNoteModel.kt (1)
152-157:⚠️ Potential issue | 🟠 Major
searchResults!!infolder.observeForeveris an NPE waiting to happen.
folderis aNotNullLiveDatawith an existing value, soobserveForeveron Line 154 dispatches immediately and synchronously. At that point,searchResultsis stillnullunlessNotallyDatabase.getDatabase(app).observeForever(::init)on Line 153 also dispatched synchronously (which it does only if the database LiveData already has a value). If the database is still initializing (possible in edge cases even after migrations complete),inithasn't run,searchResultsisnull, andsearchResults!!.fetch(...)crashes.Swap
!!for a safe call:🛡️ Proposed fix
folder.observeForever { newFolder -> - searchResults!!.fetch(keyword, newFolder, currentLabel) + searchResults?.fetch(keyword, newFolder, currentLabel) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt` around lines 152 - 157, In startObserving(), change the unsafe non-null assertion on searchResults inside folder.observeForever to a safe null check: ensure that when folder.observeForever triggers (in BaseNoteModel::startObserving), you only call searchResults.fetch(keyword, newFolder, currentLabel) if searchResults is non-null (e.g., guard with an if (searchResults != null) or use safe-call), so that asynchronous initialization via NotallyDatabase.getDatabase(app).observeForever(::init) and init() can complete without causing an NPE on searchResults.
🧹 Nitpick comments (3)
app/src/main/java/com/philkes/notallyx/utils/MiscExtensions.kt (1)
115-117: LGTM — optional: consider@JvmInline value classfor stronger type safety.The arithmetic is correct (
abs(Long - Long) / 1000). Thetypealiasapproach is idiomatic for lightweight semantic documentation, but it provides no compile-time protection: anyLong(e.g., a byte count, duration, or plain integer) can be passed tosecondsBetweenwithout a warning.If misuse across the codebase is a concern, a
@JvmInline value classwould enforce the distinction at the type level with zero runtime overhead:♻️ Optional: value class alternative
-typealias TimeMillis = Long +@JvmInline +value class TimeMillis(val value: Long)Note: adopting this would require wrapping/unwrapping at every call site that bridges with system APIs returning plain
Long(e.g.,System.currentTimeMillis()), so the tradeoff may not be worth it unless type safety is a priority here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/MiscExtensions.kt` around lines 115 - 117, Replace the typealias TimeMillis = Long with a `@JvmInline` value class to get compile-time type safety: define `@JvmInline` value class TimeMillis(val ms: Long) and update the extension/function secondsBetween to use the new wrapper (e.g., fun TimeMillis.secondsBetween(other: TimeMillis): Long = abs(this.ms - other.ms) / 1000), and then call sites that use raw Longs (e.g., System.currentTimeMillis()) should be wrapped/unwrapped explicitly (TimeMillis(System.currentTimeMillis())).app/src/main/java/com/philkes/notallyx/utils/DataSchemaMigrations.kt (1)
90-97: Delete-on-failure is a reasonable last resort, but consider broader exception handling.The nested try-catch properly handles a persistent
SQLiteBlobTooBigExceptionby deleting the unrecoverable note. However,truncateBodyAndFixSpans(line 91) could also throw other exceptions (e.g., genericSQLExceptionfromdao.truncateBodyordao.insert). An unexpected exception here would abort the entire migration loop, leaving remaining notes unprocessed.Consider catching a broader exception type (e.g.,
Exception) on the outer try (lines 90-97) or at minimumSQLiteException, so one corrupted row doesn't halt processing of the rest.Also a minor nit: the inner
e(line 93) shadows the outere(line 82). Renaming one (e.g.,e2) would improve clarity.Proposed diff
- try { - truncateBodyAndFixSpans(dao, id) - dao.get(id) - } catch (e: SQLiteBlobTooBigException) { - log(TAG, "Note (id: $id) could not be repaired. Deleting...", e) + try { + truncateBodyAndFixSpans(dao, id) + dao.get(id) + } catch (e2: SQLiteBlobTooBigException) { + log(TAG, "Note (id: $id) could not be repaired. Deleting...", e2) dao.delete(id) null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/DataSchemaMigrations.kt` around lines 90 - 97, The current try-catch around truncateBodyAndFixSpans(dao, id) only catches SQLiteBlobTooBigException and uses a nested exception variable that shadows the outer one; update the error handling so the outer try (surrounding truncateBodyAndFixSpans and dao.get) also catches a broader exception type (e.g., Exception or SQLiteException) to prevent one bad row from aborting the migration loop, and rename the inner catch variable (e.g., change the inner catch from "e" to "e2") to avoid shadowing the outer "e" defined earlier; locate and modify the block around truncateBodyAndFixSpans, dao.get, and dao.delete to implement these changes and log the broader exception appropriately before continuing.app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt (1)
296-312:SQLiteBlobTooBigExceptionfallback is unreachable dead code; also, redundant fully-qualified reference at line 304.The
bodycolumn inbaseNoteCursoris explicitly capped viaSUBSTR(body, 1, ${MAX_BODY_CHAR_LENGTH})in the projection (line 136), whereMAX_BODY_CHAR_LENGTH = 393,216 characters(~1.57 MB worst-case UTF-8). This pre-truncation in the cursor meansgetString(getColumnIndexOrThrow("body"))at line 297 will not throwSQLiteBlobTooBigException—the catch block at lines 296–312 is unreachable dead code.Line 304 uses the fully-qualified name
com.philkes.notallyx.data.dao.BaseNoteDao.Companion.MAX_BODY_CHAR_LENGTH, which duplicates the import at line 18.♻️ Proposed cleanup
Remove the dead catch block:
- val body = - try { - getString(getColumnIndexOrThrow("body")) - } catch (_: SQLiteBlobTooBigException) { - // Fall back to truncated read from source DB to avoid cursor window overflow - val cursor = - sourceDb.rawQuery( - "SELECT substr(body, 1, ?) AS body FROM BaseNote WHERE id = ?", - arrayOf( - com.philkes.notallyx.data.dao.BaseNoteDao.Companion.MAX_BODY_CHAR_LENGTH - .toString(), - id.toString(), - ), - ) - val value = if (cursor.moveToFirst()) cursor.getString(0) else "" - cursor.close() - value - } + val body = getString(getColumnIndexOrThrow("body"))Also remove the unused
sourceDbparameter from thetoBaseNotefunction signature.Alternatively, if the catch block is retained as defensive code, at minimum fix the redundant FQ reference:
- com.philkes.notallyx.data.dao.BaseNoteDao.Companion.MAX_BODY_CHAR_LENGTH + MAX_BODY_CHAR_LENGTH🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt` around lines 296 - 312, The catch for SQLiteBlobTooBigException in toBaseNote is dead because baseNoteCursor already projects SUBSTR(body, 1, MAX_BODY_CHAR_LENGTH), so remove the try/catch fallback and its rawQuery usage (including the redundant sourceDb dependency); also replace the fully-qualified reference com.philkes.notallyx.data.dao.BaseNoteDao.Companion.MAX_BODY_CHAR_LENGTH with the existing imported MAX_BODY_CHAR_LENGTH or the symbol BaseNoteDao.Companion.MAX_BODY_CHAR_LENGTH if needed, and then remove the now-unused sourceDb parameter from the toBaseNote signature and all call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/LockedActivity.kt`:
- Around line 76-84: The process-wide uncaught-exception handler installed via
Thread.setDefaultUncaughtExceptionHandler in LockedActivity captures this and
then launches on lifecycleScope (which may be cancelled if the activity is
destroyed), so move the recovery work to an application-scoped coroutine context
(e.g., a GlobalScope or an Application-level CoroutineScope) instead of
lifecycleScope, keep using EXCEPTION_HANDLER_MUTEX and
isExceptionAlreadyBeingHandled as before, and before performing any UI work
(showing the progress dialog or interacting with this@LockedActivity) check the
activity is still valid/alive (e.g., isFinishing/isDestroyed or a safe flag) so
UI is only touched when safe.
- Around line 80-84: Reorder the conditional so the exception type is checked
first and only then apply the dedup guard: first test if throwable is a
SQLiteBlobTooBigException (or has a cause of that type) using the existing
throwable and SQLiteBlobTooBigException checks, and if it matches then consult
isExceptionAlreadyBeingHandled(time) to decide whether to handle it locally or
delegate to the previous uncaught-exception handler; update the branch that
currently contains isExceptionAlreadyBeingHandled(time) so that legitimate
SQLiteBlobTooBigException occurrences are not skipped when dedup returns false
and are properly forwarded to the previous handler when appropriate.
- Around line 75-109: Capture the existing default handler before replacing it
in setupGlobalExceptionHandler (val previousHandler =
Thread.getDefaultUncaughtExceptionHandler()), and then in the new
Thread.setDefaultUncaughtExceptionHandler implementation, only take the
coroutine-based migration path for handled SQLiteBlobTooBigException cases and
synchronously delegate any other throwables to previousHandler. Specifically, in
setupGlobalExceptionHandler keep the current coroutine logic for when (throwable
is SQLiteBlobTooBigException || throwable.cause is SQLiteBlobTooBigException)
but for the else branch call previousHandler?.uncaughtException(thread,
throwable) (or previousHandler?.uncaughtException(thread, throwable) after
returning from coroutine if you must run async work), ensuring you reference
setupGlobalExceptionHandler, Thread.setDefaultUncaughtExceptionHandler, and
previousHandler so non-handled exceptions are not swallowed.
- Around line 111-112: Rename the function isExceptionAlreadyBeingHandled to
reflect its actual semantics (returns true when the exception is NOT currently
being handled), e.g., shouldHandleException(time: Long) or
isExceptionNotBeingHandled(time: Long); update the implementation that reads
EXCEPTION_HANDLER_MUTEX_LAST_TIMESTAMP?.let { it.secondsBetween(time) > 20 } ?:
true to the new name without changing logic, then update all call sites
(including the usage that currently checks the condition around
EXCEPTION_HANDLER_MUTEX_LAST_TIMESTAMP) to invoke the new function name so the
condition reads clearly (e.g., if (shouldHandleException(time)) { ... }).
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 900-903: The catch block in BaseNoteModel.kt currently uses the
Kotlin 2.0 discard syntax `catch (_: Exception)` which won't compile under
Kotlin 1.9.0; change the catch to use a named parameter (for example `catch (e:
Exception)`) and keep the existing body that sets
showRefreshBackupsFolderAfterThemeChange = false and calls disableBackups(), or
alternatively bump the project Kotlin version to 2.0+ if you prefer to use
discard syntax.
- Around line 117-119: deletedNotes and archivedNotes are pre-initialized with
an identity transform so their transform never becomes ::transform during init,
causing section headers to be missing; update their initialization to use the
same transform as baseNotes. Locate the properties deletedNotes and
archivedNotes (currently Content(MutableLiveData(), { t -> t })) and change
their initializer to use ::transform like baseNotes does so that when init calls
setObserver(baseNoteDao.getFrom(...)) the Content keeps the correct transform;
ensure this mirrors the behavior of labelsHiddenObserver/initBaseNotes which
sets baseNotes = null before init.
---
Outside diff comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 152-157: In startObserving(), change the unsafe non-null assertion
on searchResults inside folder.observeForever to a safe null check: ensure that
when folder.observeForever triggers (in BaseNoteModel::startObserving), you only
call searchResults.fetch(keyword, newFolder, currentLabel) if searchResults is
non-null (e.g., guard with an if (searchResults != null) or use safe-call), so
that asynchronous initialization via
NotallyDatabase.getDatabase(app).observeForever(::init) and init() can complete
without causing an NPE on searchResults.
---
Nitpick comments:
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt`:
- Around line 296-312: The catch for SQLiteBlobTooBigException in toBaseNote is
dead because baseNoteCursor already projects SUBSTR(body, 1,
MAX_BODY_CHAR_LENGTH), so remove the try/catch fallback and its rawQuery usage
(including the redundant sourceDb dependency); also replace the fully-qualified
reference
com.philkes.notallyx.data.dao.BaseNoteDao.Companion.MAX_BODY_CHAR_LENGTH with
the existing imported MAX_BODY_CHAR_LENGTH or the symbol
BaseNoteDao.Companion.MAX_BODY_CHAR_LENGTH if needed, and then remove the
now-unused sourceDb parameter from the toBaseNote signature and all call sites.
In `@app/src/main/java/com/philkes/notallyx/utils/DataSchemaMigrations.kt`:
- Around line 90-97: The current try-catch around truncateBodyAndFixSpans(dao,
id) only catches SQLiteBlobTooBigException and uses a nested exception variable
that shadows the outer one; update the error handling so the outer try
(surrounding truncateBodyAndFixSpans and dao.get) also catches a broader
exception type (e.g., Exception or SQLiteException) to prevent one bad row from
aborting the migration loop, and rename the inner catch variable (e.g., change
the inner catch from "e" to "e2") to avoid shadowing the outer "e" defined
earlier; locate and modify the block around truncateBodyAndFixSpans, dao.get,
and dao.delete to implement these changes and log the broader exception
appropriately before continuing.
In `@app/src/main/java/com/philkes/notallyx/utils/MiscExtensions.kt`:
- Around line 115-117: Replace the typealias TimeMillis = Long with a `@JvmInline`
value class to get compile-time type safety: define `@JvmInline` value class
TimeMillis(val ms: Long) and update the extension/function secondsBetween to use
the new wrapper (e.g., fun TimeMillis.secondsBetween(other: TimeMillis): Long =
abs(this.ms - other.ms) / 1000), and then call sites that use raw Longs (e.g.,
System.currentTimeMillis()) should be wrapped/unwrapped explicitly
(TimeMillis(System.currentTimeMillis())).
Comment on lines
+111
to
+112
| private fun isExceptionAlreadyBeingHandled(time: Long): Boolean = | ||
| EXCEPTION_HANDLER_MUTEX_LAST_TIMESTAMP?.let { it.secondsBetween(time) > 20 } ?: true |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Method name has inverted semantics — rename for clarity.
isExceptionAlreadyBeingHandled returns true on the first call (null timestamp) and when >20s have elapsed — i.e., when the exception is not currently being handled. The name suggests the opposite. This makes the condition on line 81 read as "if exception IS already being handled, handle it again," which is confusing.
Proposed rename
- private fun isExceptionAlreadyBeingHandled(time: Long): Boolean =
- EXCEPTION_HANDLER_MUTEX_LAST_TIMESTAMP?.let { it.secondsBetween(time) > 20 } ?: true
+ private fun canHandleException(time: Long): Boolean =
+ EXCEPTION_HANDLER_MUTEX_LAST_TIMESTAMP?.let { it.secondsBetween(time) > 20 } ?: trueAnd update the call site accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/LockedActivity.kt`
around lines 111 - 112, Rename the function isExceptionAlreadyBeingHandled to
reflect its actual semantics (returns true when the exception is NOT currently
being handled), e.g., shouldHandleException(time: Long) or
isExceptionNotBeingHandled(time: Long); update the implementation that reads
EXCEPTION_HANDLER_MUTEX_LAST_TIMESTAMP?.let { it.secondsBetween(time) > 20 } ?:
true to the new name without changing logic, then update all call sites
(including the usage that currently checks the condition around
EXCEPTION_HANDLER_MUTEX_LAST_TIMESTAMP) to invoke the new function name so the
condition reads clearly (e.g., if (shouldHandleException(time)) { ... }).
Comment on lines
+900
to
903
| } catch (_: Exception) { | ||
| showRefreshBackupsFolderAfterThemeChange = false | ||
| disableBackups() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for Kotlin version declarations in gradle files and version catalogs
echo "=== Searching for Kotlin version in gradle files ==="
fd -e toml -e gradle -e "gradle.kts" | xargs grep -i "kotlin" 2>/dev/null | grep -iE "version|kotlin-version|kotlinVersion|kotlin-gradle-plugin" | head -40Repository: Crustack/NotallyX
Length of output: 288
🏁 Script executed:
#!/bin/bash
# More comprehensive search for all Kotlin version configurations
echo "=== All Kotlin version-related declarations ==="
rg -i "kotlin.*version|version.*kotlin" --type gradle --type toml -A 2 -B 2 | head -60Repository: Crustack/NotallyX
Length of output: 274
🏁 Script executed:
#!/bin/bash
# Broader search for Kotlin version
echo "=== All references to Kotlin 1.9 and 2.0 ==="
rg -i "kotlin.*1\.9|1\.9.*kotlin|kotlin.*2\.0|2\.0.*kotlin" | head -20
echo ""
echo "=== Content of main build files ==="
for file in build.gradle.kts app/build.gradle.kts gradle/libs.versions.toml; do
if [ -f "$file" ]; then
echo "File: $file"
cat "$file" | grep -i "kotlin\|version" | head -15
echo ""
fi
doneRepository: Crustack/NotallyX
Length of output: 1295
Use of catch (_: Exception) is incompatible with this project's Kotlin version 1.9.0.
The discard syntax for exception parameters (catch (_: Exception)) was introduced in Kotlin 2.0. This project targets Kotlin 1.9.0, where this syntax will fail to compile. Either update the Kotlin version to 2.0 or later, or use a named parameter instead (e.g., catch (_: Exception) → catch (e: Exception)).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`
around lines 900 - 903, The catch block in BaseNoteModel.kt currently uses the
Kotlin 2.0 discard syntax `catch (_: Exception)` which won't compile under
Kotlin 1.9.0; change the catch to use a named parameter (for example `catch (e:
Exception)`) and keep the existing body that sets
showRefreshBackupsFolderAfterThemeChange = false and calls disableBackups(), or
alternatively bump the project Kotlin version to 2.0+ if you prefer to use
discard syntax.
Crustack
force-pushed
the
fix/data-migrations
branch
from
68c6cae to
0fffc9c
Compare
Contributor
There was a problem hiding this comment.
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/viewmodel/BaseNoteModel.kt (1)
152-157:⚠️ Potential issue | 🟠 MajorPotential NPE:
searchResults!!on line 155 if the database LiveData hasn't dispatched yet.
folderalready has a value (Folder.NOTES), soobserveForeveron line 154 dispatches the observer callback immediately. IfNotallyDatabase.getDatabase(app)on line 153 has not yet emitted a value (database still initializing),init()hasn't run andsearchResultsis stillnull— causingsearchResults!!.fetch(...)to throwNullPointerException.This depends on whether the database
LiveDatasynchronously dispatches its cached value uponobserveForever. If it does,init()runs first andsearchResultsis set. If it doesn't (e.g., first cold start), this is a crash.#!/bin/bash # Check how NotallyDatabase.getDatabase returns its LiveData - does it have an initial value? echo "=== NotallyDatabase.getDatabase implementation ===" ast-grep --pattern $'fun getDatabase($$$): $_ { $$$ }' echo "" echo "=== Search for getDatabase companion/factory ===" rg -n "fun getDatabase" --type kotlin -A 20 echo "" echo "=== Check if the returned LiveData is pre-populated ===" rg -n "MutableLiveData|NotNullLiveData" --type kotlin -g '*NotallyDatabase*' -B2 -A2Suggested defensive fix
fun startObserving() { NotallyDatabase.getDatabase(app).observeForever(::init) folder.observeForever { newFolder -> - searchResults!!.fetch(keyword, newFolder, currentLabel) + searchResults?.fetch(keyword, newFolder, currentLabel) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt` around lines 152 - 157, startObserving can call searchResults!! before init() runs because NotallyDatabase.getDatabase(app).observeForever(::init) may not dispatch immediately; change startObserving to guard against a null searchResults by making the folder observer check searchResults != null (or early-return) before calling searchResults.fetch, or defer attaching the folder.observeForever until after init has set searchResults; reference functions/vars: startObserving, NotallyDatabase.getDatabase(app), init, folder.observeForever, and searchResults.
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
181-191: Deadnullbranches:deletedNotesandarchivedNotesare nevernull.Since both are pre-initialized on lines 118–119 and never set to
null, theif (deletedNotes == null)andif (archivedNotes == null)conditions are alwaysfalse. Theifbranches (lines 182, 188) are unreachable.Consider removing the dead branches for clarity, or if the
nullpath is meant as a safety net, add a comment explaining the intent.Simplified version
- if (deletedNotes == null) { - deletedNotes = Content(baseNoteDao.getFrom(Folder.DELETED), ::transform) - } else { - deletedNotes!!.setObserver(baseNoteDao.getFrom(Folder.DELETED)) - } + deletedNotes!!.setObserver(baseNoteDao.getFrom(Folder.DELETED)) - if (archivedNotes == null) { - archivedNotes = Content(baseNoteDao.getFrom(Folder.ARCHIVED), ::transform) - } else { - archivedNotes!!.setObserver(baseNoteDao.getFrom(Folder.ARCHIVED)) - } + archivedNotes!!.setObserver(baseNoteDao.getFrom(Folder.ARCHIVED))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt` around lines 181 - 191, The null-check branches for deletedNotes and archivedNotes are dead because deletedNotes and archivedNotes are pre-initialized and never set to null; remove the unreachable if (deletedNotes == null) / if (archivedNotes == null) blocks and simply call deletedNotes!!.setObserver(baseNoteDao.getFrom(Folder.DELETED)) and archivedNotes!!.setObserver(baseNoteDao.getFrom(Folder.ARCHIVED)) (or better, make them non-nullable and drop the !!), or if you intended a safety-net, replace the dead branches with a concise comment explaining why a null-path would be expected and keep one clear path that sets the observer on the existing Content instances (symbols: deletedNotes, archivedNotes, Content, baseNoteDao.getFrom(Folder.DELETED), baseNoteDao.getFrom(Folder.ARCHIVED)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 152-157: startObserving can call searchResults!! before init()
runs because NotallyDatabase.getDatabase(app).observeForever(::init) may not
dispatch immediately; change startObserving to guard against a null
searchResults by making the folder observer check searchResults != null (or
early-return) before calling searchResults.fetch, or defer attaching the
folder.observeForever until after init has set searchResults; reference
functions/vars: startObserving, NotallyDatabase.getDatabase(app), init,
folder.observeForever, and searchResults.
---
Duplicate comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/LockedActivity.kt`:
- Around line 75-113: The global handler in setupGlobalExceptionHandler
currently captures this@LockedActivity and uses lifecycleScope (so recovery can
be cancelled and leaked across onCreate calls); fix it by installing the handler
only once (guard setupGlobalExceptionHandler with a static/companion boolean)
and move the recovery work out of lifecycleScope into an application-scoped
CoroutineScope (create/use an Application-level CoroutineScope or GlobalScope
with Dispatchers.IO) when launching the coroutine that calls
application.splitOversizedNotes() and uses EXCEPTION_HANDLER_MUTEX; do not hold
strong references to LockedActivity — only keep a WeakReference if you must
reference the activity for UI work; guard any UI calls to setupProgressDialog /
posting MigrationProgress with activity?.isFinishing/isDestroyed checks and
perform them on the main thread (or skip and call
previousHandler?.uncaughtException(thread, throwable)) so that if the activity
is gone the exception is delegated to previousHandler.
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 900-903: The catch uses Kotlin 2.0 discard syntax "catch (_:
Exception)" which will not compile on Kotlin 1.9.x; update the catch to use an
explicit exception parameter (e.g., "catch (e: Exception)") or otherwise remove
the parameter and handle/log e, in the BaseNoteModel.kt block that sets
showRefreshBackupsFolderAfterThemeChange = false and calls disableBackups(),
and/or bump the project Kotlin version to 2.0+ in your Gradle/libs config;
ensure any references to the exception variable (if added) are either used or
explicitly ignored per your lint rules.
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 181-191: The null-check branches for deletedNotes and
archivedNotes are dead because deletedNotes and archivedNotes are
pre-initialized and never set to null; remove the unreachable if (deletedNotes
== null) / if (archivedNotes == null) blocks and simply call
deletedNotes!!.setObserver(baseNoteDao.getFrom(Folder.DELETED)) and
archivedNotes!!.setObserver(baseNoteDao.getFrom(Folder.ARCHIVED)) (or better,
make them non-nullable and drop the !!), or if you intended a safety-net,
replace the dead branches with a concise comment explaining why a null-path
would be expected and keep one clear path that sets the observer on the existing
Content instances (symbols: deletedNotes, archivedNotes, Content,
baseNoteDao.getFrom(Folder.DELETED), baseNoteDao.getFrom(Folder.ARCHIVED)).
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.
Fixes #837
Summary by CodeRabbit
Bug Fixes
Improvements