Move attachment files according to dataInPublicFolder setting#793
Merged
Move attachment files according to dataInPublicFolder setting#793
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds privacy-aware, mode-selectable attachment storage with current/alternate directory resolvers, migration tooling run at startup, a BroadcastReceiver + WorkManager CoroutineWorker to clean missing attachments, and updates codepaths to use new storage/resolution helpers and notify about missing backup attachments. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Prefs as NotallyXPreferences
participant IO as IOExtensions
participant DB as Database
participant Scope as MainScope/Coroutine
App->>Prefs: read dataSchemaId
alt dataSchemaId < 1
App->>Prefs: read privacy mode
App->>Scope: launch IO coroutine
Scope->>IO: migrateAllAttachments(toPrivate?)
IO->>IO: iterate/copy files between roots
IO->>DB: (no direct schema changes) update moved state if needed
Scope->>Prefs: setDataSchemaId(1)
else
App->>App: no-op
end
sequenceDiagram
participant System as Broadcast System
participant Receiver as CleanupMissingAttachmentsReceiver
participant WM as WorkManager
participant Worker as CleanupMissingAttachmentsWorker
participant DB as Database
participant IO as IOExtensions
participant Notif as NotificationManager
System->>Receiver: send CLEANUP_MISSING_ATTACHMENTS
Receiver->>WM: enqueueUniqueWork(CleanupMissingAttachmentsWorker)
WM->>Worker: start doWork()
Worker->>DB: load all notes with attachments
Worker->>IO: resolve attachment files (current/alternate)
Worker->>Worker: detect missing files, update note attachments
Worker->>DB: persist updated notes
Worker->>Notif: post completion notification(summary)
Worker->>WM: return Result.success()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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
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 (2)
app/src/main/java/com/philkes/notallyx/presentation/activity/note/ViewImageActivity.kt (1)
232-248: Success toast shown even when file write fails.The toast at lines 246-247 is shown unconditionally after the coroutine completes. If
resolveAttachmentFilereturnsnullor the file doesn't exist, no data is written but the user still sees "Saved to device". With the new attachment resolution logic that may returnnullwhen files are missing (per PR objectives around missing attachments), this becomes more likely to mislead users.Suggested fix
private fun writeImageToUri(uri: Uri) { lifecycleScope.launch { - withContext(Dispatchers.IO) { + val success = withContext(Dispatchers.IO) { val ci = requireNotNull(currentImage) { "currentImage is null" } val file = application.resolveAttachmentFile(SUBFOLDER_IMAGES, ci.localName) if (file != null && file.exists()) { val output = contentResolver.openOutputStream(uri) as FileOutputStream output.channel.truncate(0) val input = FileInputStream(file) input.copyTo(output) input.close() output.close() + true + } else { + false } } - Toast.makeText(this@ViewImageActivity, R.string.saved_to_device, Toast.LENGTH_LONG) - .show() + if (success) { + Toast.makeText(this@ViewImageActivity, R.string.saved_to_device, Toast.LENGTH_LONG) + .show() + } else { + Toast.makeText(this@ViewImageActivity, R.string.error_saving_file, Toast.LENGTH_LONG) + .show() + } } }app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt (1)
212-233: Potential NullPointerException when directory is null.
getCurrentImagesDirectory(),getCurrentFilesDirectory(), andgetCurrentAudioDirectory()can returnnull(as seen inIOExtensions.kt). PassingnulltoFile(parent, child)constructor will use current working directory, which is likely unintended and could cause the backup to reference non-existent files.Proposed fix
val files = with(changedNote) { - images.map { - BackupFile( - SUBFOLDER_IMAGES, - File(getCurrentImagesDirectory(), it.localName), - ) - } + - files.map { - BackupFile( - SUBFOLDER_FILES, - File(getCurrentFilesDirectory(), it.localName), - ) - } + - audios.map { - BackupFile( - SUBFOLDER_AUDIOS, - File(getCurrentAudioDirectory(), it.name), - ) - } + + val imgDir = getCurrentImagesDirectory() + val fileDir = getCurrentFilesDirectory() + val audioDir = getCurrentAudioDirectory() + images.mapNotNull { img -> + imgDir?.let { BackupFile(SUBFOLDER_IMAGES, File(it, img.localName)) } + } + + files.mapNotNull { f -> + fileDir?.let { BackupFile(SUBFOLDER_FILES, File(it, f.localName)) } + } + + audios.mapNotNull { aud -> + audioDir?.let { BackupFile(SUBFOLDER_AUDIOS, File(it, aud.name)) } + } + BackupFile(null, databaseFile) }
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteVH.kt`:
- Around line 296-331: The ImageView is set VISIBLE before Glide loads which can
leave a stale image when a load fails or file is missing; update the Glide
request listener (in BaseNoteVH.kt where ImageView, Message and the
Glide.load(file) block are used) to call Glide.with(ImageView).clear(ImageView)
and set ImageView.visibility = GONE in onLoadFailed, ensure Message.visibility =
VISIBLE there, and in the else branch (file missing) also clear and hide
ImageView before showing Message; additionally, in onResourceReady set
Message.visibility = GONE and ensure ImageView stays VISIBLE so successful loads
and failures consistently update visibility and clear any recycled bitmaps.
In `@app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt`:
- Around line 394-398: The code builds filesUris by calling
File(getCurrentImagesDirectory(), it.localName) but getCurrentImagesDirectory()
returns File? and can be null; update the logic in the scope where filesUris is
built (referencing getCurrentImagesDirectory(), File(...), getUriForFile(),
filesUris, and shareNote) to explicitly handle a null images directory: either
obtain the non-null directory first and bail/skip when it's null, or filter
note.images to only map when getCurrentImagesDirectory() is non-null, then
create Files and call getUriForFile() only for those valid Files; ensure
shareNote is called with the filtered/empty filesUris accordingly.
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt`:
- Around line 148-150: The variables imageRoot, fileRoot, and audioRoot are
assigned from nullable functions getCurrentImagesDirectory(),
getCurrentFilesDirectory(), and getCurrentAudioDirectory() but later get forced
with !!; update the assignments to validate them (e.g., use requireNotNull or
explicit null checks) with descriptive messages like "Images directory missing"
/ "Files directory missing" / "Audio directory missing" so the import functions
fail fast with clear errors; locate usages of imageRoot, fileRoot, and audioRoot
in ImportExtensions.kt (they are later dereferenced) and replace the unsafe !!
assumptions with the requireNotNull guards consistent with the other import
functions in this file.
In `@app/src/main/java/com/philkes/notallyx/utils/DataSchemaMigrations.kt`:
- Around line 16-21: The code currently calls
preferences.setDataSchemaId(newDataSchemaId) before
moveAttachments/migrateAllAttachments finishes, and writes even when no change
is needed; fix by moving the preferences.setDataSchemaId call into the coroutine
path after migrateAllAttachments completes successfully (or replace the
coroutine with runBlocking if you must block), e.g. in
moveAttachments/migrateAllAttachments success handler set the schema id there
using preferences.setDataSchemaId(newDataSchemaId), and guard the write so it
only occurs when dataSchemaId != newDataSchemaId; also ensure exceptions from
migrateAllAttachments are propagated/handled so the schema id is not written on
failure.
In `@app/src/main/java/com/philkes/notallyx/utils/IOExtensions.kt`:
- Around line 73-80: KDoc and implementation diverge for
ContextWrapper.isPrivateAttachmentMode(): either update the KDoc to remove the
biometric-lock mention or adjust the function to also check the biometric
preference in NotallyXPreferences (e.g., include
prefs.<biometricPreference>.value && !prefs.dataInPublicFolder.value). Locate
isPrivateAttachmentMode, NotallyXPreferences and dataInPublicFolder in
IOExtensions.kt and apply the chosen fix so the comment accurately reflects the
runtime behavior.
- Around line 142-209: The
migrateAllAttachments(ContextWrapper.migrateAllAttachments) logic currently
calls file.copyTo(target, overwrite = true) which contradicts the intent in the
comment to "skip copy and delete source" when a target exists and risks
clobbering data; change the routine to first check for target.exists() (or
compare file sizes/checksums if you want to handle conflicts) and if the target
exists skip copying and still delete the source (or skip delete if you prefer to
preserve both), otherwise perform a non-overwriting copy; also update the inline
comment to reflect the chosen behavior (skip or compare) so the code and comment
match.
🧹 Nitpick comments (6)
app/src/main/java/com/philkes/notallyx/presentation/view/note/preview/PreviewImageVH.kt (1)
26-29: Consider clearing the ImageView when file doesn't exist.When this ViewHolder is recycled from a previous valid image, the early return leaves the old image in
binding.ImageView. Depending on the layout, users might see a stale image alongside the visible error message.♻️ Suggested fix
if (file?.exists() == false) { binding.Message.visibility = View.VISIBLE + binding.ImageView.setImageDrawable(null) return }app/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.kt (1)
132-149: Existing crash risk withrequireNotNullon directory accessors.If
getCurrentImagesDirectory()orgetCurrentFilesDirectory()returnsnull(e.g., storage unavailable),requireNotNullwill throw anIllegalArgumentException, crashing the app. This is pre-existing behavior, but consider gracefully handling this scenario by showing an error toast and aborting the operation instead.Example graceful handling
fun addImages(uris: Array<Uri>) { imageRoot = app.getCurrentImagesDirectory() - requireNotNull(imageRoot) { "imageRoot is null" } - addFiles(uris, imageRoot!!, FileType.IMAGE) + if (imageRoot == null) { + app.showToast(R.string.insert_an_sd_card_images) + return + } + addFiles(uris, imageRoot!!, FileType.IMAGE) }app/src/main/java/com/philkes/notallyx/utils/backup/CleanupMissingAttachmentsWorker.kt (2)
63-77: Consider wrapping updates in a database transaction.Multiple individual DAO updates per note (lines 67, 71, 75) without a transaction could leave the database in an inconsistent state if the app crashes mid-cleanup. Additionally, this creates unnecessary overhead for notes with multiple attachment types missing.
Proposed fix using transaction
if (imgRemoved + fileRemoved + audRemoved > 0) { affectedNotes++ - if (imgRemoved > 0) { - removedImages += imgRemoved - dao.updateImages(note.id, filteredImages) - } - if (fileRemoved > 0) { - removedFiles += fileRemoved - dao.updateFiles(note.id, filteredFiles) - } - if (audRemoved > 0) { - removedAudios += audRemoved - dao.updateAudios(note.id, filteredAudios) - } + database.withTransaction { + if (imgRemoved > 0) { + removedImages += imgRemoved + dao.updateImages(note.id, filteredImages) + } + if (fileRemoved > 0) { + removedFiles += fileRemoved + dao.updateFiles(note.id, filteredFiles) + } + if (audRemoved > 0) { + removedAudios += audRemoved + dao.updateAudios(note.id, filteredAudios) + } + } }
26-83: Add error handling to prevent silent failures.The
doWork()method has no try-catch around the main processing loop. If an exception occurs (e.g., database access fails), it will propagate and the worker will fail without proper logging or user notification. Consider wrapping the logic and returningResult.failure()on errors.Proposed fix
override suspend fun doWork(): Result { + return try { + performCleanup() + } catch (e: Exception) { + val ctx = ContextWrapper(applicationContext) + ctx.log(TAG, "Cleanup failed", e) + Result.failure() + } + } + + private suspend fun performCleanup(): Result { val ctx = ContextWrapper(applicationContext) // ... existing logic ... + return Result.success() + }app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
280-280: Migration result is discarded.Both
migrateAllAttachmentscalls return aPair<Int, Int>(moved, failed) that is currently ignored. Consider logging or notifying the user if any files failed to migrate, to help diagnose issues.Example logging
- app.migrateAllAttachments(toPrivate = false) + val (moved, failed) = app.migrateAllAttachments(toPrivate = false) + if (failed > 0) { + app.log("BaseNoteModel", "Migration: $moved moved, $failed failed") + }Also applies to: 303-303
app/src/main/java/com/philkes/notallyx/utils/IOExtensions.kt (1)
49-71: Harden private directory creation.
mkdir()can fail silently when a file exists at that path or if the parent isn’t created, which will later break migrations. Considermkdirs()plus a file-collision guard.♻️ Proposed fix
fun ContextWrapper.getPrivateAttachmentsRoot(): File { val root = File(filesDir, "attachments") - if (!root.exists()) root.mkdir() + if (root.exists() && !root.isDirectory) { + root.delete() + } + if (!root.exists()) root.mkdirs() return root } fun ContextWrapper.getPrivateImagesDirectory(): File { val dir = File(getPrivateAttachmentsRoot(), SUBFOLDER_IMAGES) - if (!dir.exists()) dir.mkdir() + if (!dir.exists()) dir.mkdirs() return dir } fun ContextWrapper.getPrivateFilesDirectory(): File { val dir = File(getPrivateAttachmentsRoot(), SUBFOLDER_FILES) - if (!dir.exists()) dir.mkdir() + if (!dir.exists()) dir.mkdirs() return dir } fun ContextWrapper.getPrivateAudioDirectory(): File { val dir = File(getPrivateAttachmentsRoot(), SUBFOLDER_AUDIOS) - if (!dir.exists()) dir.mkdir() + if (!dir.exists()) dir.mkdirs() return dir }
Comment on lines
+296
to
+331
| if (file?.exists() == true) { | ||
| ImageView.visibility = VISIBLE | ||
| Glide.with(ImageView) | ||
| .load(file) | ||
| .centerCrop() | ||
| .transition(DrawableTransitionOptions.withCrossFade()) | ||
| .diskCacheStrategy(DiskCacheStrategy.NONE) | ||
| .listener( | ||
| object : RequestListener<Drawable> { | ||
|
|
||
| override fun onLoadFailed( | ||
| e: GlideException?, | ||
| model: Any?, | ||
| target: Target<Drawable>?, | ||
| isFirstResource: Boolean, | ||
| ): Boolean { | ||
| Message.visibility = VISIBLE | ||
| return false | ||
| } | ||
|
|
||
| Glide.with(ImageView) | ||
| .load(file) | ||
| .centerCrop() | ||
| .transition(DrawableTransitionOptions.withCrossFade()) | ||
| .diskCacheStrategy(DiskCacheStrategy.NONE) | ||
| .listener( | ||
| object : RequestListener<Drawable> { | ||
|
|
||
| override fun onLoadFailed( | ||
| e: GlideException?, | ||
| model: Any?, | ||
| target: Target<Drawable>?, | ||
| isFirstResource: Boolean, | ||
| ): Boolean { | ||
| Message.visibility = VISIBLE | ||
| return false | ||
| } | ||
|
|
||
| override fun onResourceReady( | ||
| resource: Drawable?, | ||
| model: Any?, | ||
| target: Target<Drawable>?, | ||
| dataSource: DataSource?, | ||
| isFirstResource: Boolean, | ||
| ): Boolean { | ||
| return false | ||
| override fun onResourceReady( | ||
| resource: Drawable?, | ||
| model: Any?, | ||
| target: Target<Drawable>?, | ||
| dataSource: DataSource?, | ||
| isFirstResource: Boolean, | ||
| ): Boolean { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| ) | ||
| .into(ImageView) | ||
| ) | ||
| .into(ImageView) | ||
| } else { | ||
| ImageView.visibility = GONE | ||
| Message.visibility = VISIBLE | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent stale image leakage on load failure/missing file.
ImageView is made visible before Glide load; if load fails, only Message is shown, which can leave a recycled image visible (privacy/UX risk). Clear + hide the image on failure and when the file is missing.
🛠️ Proposed fix
override fun onLoadFailed(
e: GlideException?,
model: Any?,
target: Target<Drawable>?,
isFirstResource: Boolean,
): Boolean {
Message.visibility = VISIBLE
+ ImageView.visibility = GONE
+ Glide.with(ImageView).clear(ImageView)
return false
}
...
} else {
+ Glide.with(ImageView).clear(ImageView)
ImageView.visibility = GONE
Message.visibility = VISIBLE
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (file?.exists() == true) { | |
| ImageView.visibility = VISIBLE | |
| Glide.with(ImageView) | |
| .load(file) | |
| .centerCrop() | |
| .transition(DrawableTransitionOptions.withCrossFade()) | |
| .diskCacheStrategy(DiskCacheStrategy.NONE) | |
| .listener( | |
| object : RequestListener<Drawable> { | |
| override fun onLoadFailed( | |
| e: GlideException?, | |
| model: Any?, | |
| target: Target<Drawable>?, | |
| isFirstResource: Boolean, | |
| ): Boolean { | |
| Message.visibility = VISIBLE | |
| return false | |
| } | |
| Glide.with(ImageView) | |
| .load(file) | |
| .centerCrop() | |
| .transition(DrawableTransitionOptions.withCrossFade()) | |
| .diskCacheStrategy(DiskCacheStrategy.NONE) | |
| .listener( | |
| object : RequestListener<Drawable> { | |
| override fun onLoadFailed( | |
| e: GlideException?, | |
| model: Any?, | |
| target: Target<Drawable>?, | |
| isFirstResource: Boolean, | |
| ): Boolean { | |
| Message.visibility = VISIBLE | |
| return false | |
| } | |
| override fun onResourceReady( | |
| resource: Drawable?, | |
| model: Any?, | |
| target: Target<Drawable>?, | |
| dataSource: DataSource?, | |
| isFirstResource: Boolean, | |
| ): Boolean { | |
| return false | |
| override fun onResourceReady( | |
| resource: Drawable?, | |
| model: Any?, | |
| target: Target<Drawable>?, | |
| dataSource: DataSource?, | |
| isFirstResource: Boolean, | |
| ): Boolean { | |
| return false | |
| } | |
| } | |
| } | |
| ) | |
| .into(ImageView) | |
| ) | |
| .into(ImageView) | |
| } else { | |
| ImageView.visibility = GONE | |
| Message.visibility = VISIBLE | |
| } | |
| if (file?.exists() == true) { | |
| ImageView.visibility = VISIBLE | |
| Glide.with(ImageView) | |
| .load(file) | |
| .centerCrop() | |
| .transition(DrawableTransitionOptions.withCrossFade()) | |
| .diskCacheStrategy(DiskCacheStrategy.NONE) | |
| .listener( | |
| object : RequestListener<Drawable> { | |
| override fun onLoadFailed( | |
| e: GlideException?, | |
| model: Any?, | |
| target: Target<Drawable>?, | |
| isFirstResource: Boolean, | |
| ): Boolean { | |
| Message.visibility = VISIBLE | |
| ImageView.visibility = GONE | |
| Glide.with(ImageView).clear(ImageView) | |
| return false | |
| } | |
| override fun onResourceReady( | |
| resource: Drawable?, | |
| model: Any?, | |
| target: Target<Drawable>?, | |
| dataSource: DataSource?, | |
| isFirstResource: Boolean, | |
| ): Boolean { | |
| return false | |
| } | |
| } | |
| ) | |
| .into(ImageView) | |
| } else { | |
| Glide.with(ImageView).clear(ImageView) | |
| ImageView.visibility = GONE | |
| Message.visibility = VISIBLE | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteVH.kt`
around lines 296 - 331, The ImageView is set VISIBLE before Glide loads which
can leave a stale image when a load fails or file is missing; update the Glide
request listener (in BaseNoteVH.kt where ImageView, Message and the
Glide.load(file) block are used) to call Glide.with(ImageView).clear(ImageView)
and set ImageView.visibility = GONE in onLoadFailed, ensure Message.visibility =
VISIBLE there, and in the else branch (file missing) also clear and hide
ImageView before showing Message; additionally, in onResourceReady set
Message.visibility = GONE and ensure ImageView stays VISIBLE so successful loads
and failures consistently update visibility and clear any recycled bitmaps.
Comment on lines
394
to
398
| val filesUris = | ||
| note.images | ||
| .map { File(getExternalImagesDirectory(), it.localName) } | ||
| .map { File(getCurrentImagesDirectory(), it.localName) } | ||
| .map { getUriForFile(it) } | ||
| shareNote(note.title, body, filesUris) |
Contributor
There was a problem hiding this comment.
Potential null-safety issue with getCurrentImagesDirectory().
getCurrentImagesDirectory() returns File?. If it returns null, the File(null, it.localName) constructor will create a file with only the filename as the path, leading to incorrect file resolution and likely a FileNotFoundException or invalid URI when calling getUriForFile().
Consider filtering out images when the directory is unavailable or handling the null case explicitly.
🐛 Proposed fix
val filesUris =
- note.images
- .map { File(getCurrentImagesDirectory(), it.localName) }
- .map { getUriForFile(it) }
+ getCurrentImagesDirectory()?.let { imagesDir ->
+ note.images
+ .map { File(imagesDir, it.localName) }
+ .filter { it.exists() }
+ .map { getUriForFile(it) }
+ } ?: emptyList()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val filesUris = | |
| note.images | |
| .map { File(getExternalImagesDirectory(), it.localName) } | |
| .map { File(getCurrentImagesDirectory(), it.localName) } | |
| .map { getUriForFile(it) } | |
| shareNote(note.title, body, filesUris) | |
| val filesUris = | |
| getCurrentImagesDirectory()?.let { imagesDir -> | |
| note.images | |
| .map { File(imagesDir, it.localName) } | |
| .filter { it.exists() } | |
| .map { getUriForFile(it) } | |
| } ?: emptyList() | |
| shareNote(note.title, body, filesUris) |
🤖 Prompt for AI Agents
In `@app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt` around
lines 394 - 398, The code builds filesUris by calling
File(getCurrentImagesDirectory(), it.localName) but getCurrentImagesDirectory()
returns File? and can be null; update the logic in the scope where filesUris is
built (referencing getCurrentImagesDirectory(), File(...), getUriForFile(),
filesUris, and shareNote) to explicitly handle a null images directory: either
obtain the non-null directory first and bail/skip when it's null, or filter
note.images to only map when getCurrentImagesDirectory() is non-null, then
create Files and call getUriForFile() only for those valid Files; ensure
shareNote is called with the filtered/empty filesUris accordingly.
Comment on lines
+148
to
+150
| val imageRoot = getCurrentImagesDirectory() | ||
| val fileRoot = getCurrentFilesDirectory() | ||
| val audioRoot = getCurrentAudioDirectory() |
Contributor
There was a problem hiding this comment.
Add null safety checks for directory variables.
The getCurrentXxxDirectory() functions return nullable File?, but these variables are later dereferenced with !! (at lines 176, 231) without validation. Other import functions in this file properly use requireNotNull with descriptive messages.
Consider adding guards to fail fast with a clear error message:
Suggested improvement
val current = AtomicInteger(1)
- val imageRoot = getCurrentImagesDirectory()
- val fileRoot = getCurrentFilesDirectory()
- val audioRoot = getCurrentAudioDirectory()
+ val imageRoot = requireNotNull(getCurrentImagesDirectory()) { "imageRoot is null" }
+ val fileRoot = requireNotNull(getCurrentFilesDirectory()) { "fileRoot is null" }
+ val audioRoot = requireNotNull(getCurrentAudioDirectory()) { "audioRoot is null" }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val imageRoot = getCurrentImagesDirectory() | |
| val fileRoot = getCurrentFilesDirectory() | |
| val audioRoot = getCurrentAudioDirectory() | |
| val current = AtomicInteger(1) | |
| val imageRoot = requireNotNull(getCurrentImagesDirectory()) { "imageRoot is null" } | |
| val fileRoot = requireNotNull(getCurrentFilesDirectory()) { "fileRoot is null" } | |
| val audioRoot = requireNotNull(getCurrentAudioDirectory()) { "audioRoot is null" } |
🤖 Prompt for AI Agents
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt`
around lines 148 - 150, The variables imageRoot, fileRoot, and audioRoot are
assigned from nullable functions getCurrentImagesDirectory(),
getCurrentFilesDirectory(), and getCurrentAudioDirectory() but later get forced
with !!; update the assignments to validate them (e.g., use requireNotNull or
explicit null checks) with descriptive messages like "Images directory missing"
/ "Files directory missing" / "Audio directory missing" so the import functions
fail fast with clear errors; locate usages of imageRoot, fileRoot, and audioRoot
in ImportExtensions.kt (they are later dereferenced) and replace the unsafe !!
assumptions with the requireNotNull guards consistent with the other import
functions in this file.
app/src/main/java/com/philkes/notallyx/utils/DataSchemaMigrations.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+73
to
+80
| /** | ||
| * Determine whether attachments should be stored privately based on preferences. Private mode when | ||
| * biometric lock is enabled AND dataInPublicFolder is false. | ||
| */ | ||
| fun ContextWrapper.isPrivateAttachmentMode(): Boolean { | ||
| val prefs = NotallyXPreferences.getInstance(this) | ||
| return !prefs.dataInPublicFolder.value | ||
| } |
Contributor
There was a problem hiding this comment.
KDoc doesn’t match behavior.
The comment says biometric lock gates private mode, but the code only checks dataInPublicFolder. Please update the KDoc or add the biometric preference check if that’s the intended behavior.
✏️ KDoc update (if biometric is not part of the rule)
/**
- * Determine whether attachments should be stored privately based on preferences. Private mode when
- * biometric lock is enabled AND dataInPublicFolder is false.
+ * Determine whether attachments should be stored privately based on preferences.
+ * Private mode is enabled when dataInPublicFolder is false.
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Determine whether attachments should be stored privately based on preferences. Private mode when | |
| * biometric lock is enabled AND dataInPublicFolder is false. | |
| */ | |
| fun ContextWrapper.isPrivateAttachmentMode(): Boolean { | |
| val prefs = NotallyXPreferences.getInstance(this) | |
| return !prefs.dataInPublicFolder.value | |
| } | |
| /** | |
| * Determine whether attachments should be stored privately based on preferences. | |
| * Private mode is enabled when dataInPublicFolder is false. | |
| */ | |
| fun ContextWrapper.isPrivateAttachmentMode(): Boolean { | |
| val prefs = NotallyXPreferences.getInstance(this) | |
| return !prefs.dataInPublicFolder.value | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/java/com/philkes/notallyx/utils/IOExtensions.kt` around lines 73
- 80, KDoc and implementation diverge for
ContextWrapper.isPrivateAttachmentMode(): either update the KDoc to remove the
biometric-lock mention or adjust the function to also check the biometric
preference in NotallyXPreferences (e.g., include
prefs.<biometricPreference>.value && !prefs.dataInPublicFolder.value). Locate
isPrivateAttachmentMode, NotallyXPreferences and dataInPublicFolder in
IOExtensions.kt and apply the chosen fix so the comment accurately reflects the
runtime behavior.
Comment on lines
+142
to
+209
| /** | ||
| * Move all attachment files between public and private storage to match current mode. If | ||
| * [toPrivate] is true, move from external app media to private dirs; else the opposite. | ||
| */ | ||
| fun ContextWrapper.migrateAllAttachments(toPrivate: Boolean): Pair<Int, Int> { | ||
| var moved = 0 | ||
| var failed = 0 | ||
| val sources = listOf(SUBFOLDER_IMAGES, SUBFOLDER_FILES, SUBFOLDER_AUDIOS) | ||
| sources.forEach { sub -> | ||
| val (srcRoot, dstRoot) = | ||
| if (toPrivate) { | ||
| val src = | ||
| when (sub) { | ||
| SUBFOLDER_IMAGES -> getExternalImagesDirectory() | ||
| SUBFOLDER_FILES -> getExternalFilesDirectory() | ||
| SUBFOLDER_AUDIOS -> getExternalAudioDirectory() | ||
| else -> null | ||
| } | ||
| val dst = | ||
| when (sub) { | ||
| SUBFOLDER_IMAGES -> getPrivateImagesDirectory() | ||
| SUBFOLDER_FILES -> getPrivateFilesDirectory() | ||
| SUBFOLDER_AUDIOS -> getPrivateAudioDirectory() | ||
| else -> null | ||
| } | ||
| Pair(src, dst) | ||
| } else { | ||
| val src = | ||
| when (sub) { | ||
| SUBFOLDER_IMAGES -> getPrivateImagesDirectory() | ||
| SUBFOLDER_FILES -> getPrivateFilesDirectory() | ||
| SUBFOLDER_AUDIOS -> getPrivateAudioDirectory() | ||
| else -> null | ||
| } | ||
| val dst = | ||
| when (sub) { | ||
| SUBFOLDER_IMAGES -> getExternalImagesDirectory() | ||
| SUBFOLDER_FILES -> getExternalFilesDirectory() | ||
| SUBFOLDER_AUDIOS -> getExternalAudioDirectory() | ||
| else -> null | ||
| } | ||
| Pair(src, dst) | ||
| } | ||
| if (srcRoot == null || dstRoot == null) return@forEach | ||
| srcRoot.listFiles()?.forEach { file -> | ||
| try { | ||
| val target = File(dstRoot, file.name) | ||
| // If same file already exists at target, skip copy and delete source | ||
| file.copyTo(target, overwrite = true) | ||
| if (file.delete()) { | ||
| moved++ | ||
| } else { | ||
| // try overwrite move on legacy devices | ||
| // if (file.renameTo(target)) moved++ else failed++ | ||
| failed++ | ||
| } | ||
| } catch (t: Throwable) { | ||
| Log.e( | ||
| TAG, | ||
| "Failed to move '${file.absolutePath}' to ${if(toPrivate) "private" else "public"} folder '${dstRoot.absolutePath}'", | ||
| t, | ||
| ) | ||
| failed++ | ||
| } | ||
| } | ||
| } | ||
| return Pair(moved, failed) | ||
| } |
Contributor
There was a problem hiding this comment.
Overwrite-on-migrate contradicts the comment and can drop data.
Line 189 says “skip copy” when target exists, but copyTo(..., overwrite = true) will overwrite. If both locations can contain a file with the same name, this may discard data. Consider skipping when target exists (or compare before overwrite), or update the comment to reflect the actual behavior.
✅ Safer guard (skip existing targets)
try {
val target = File(dstRoot, file.name)
- // If same file already exists at target, skip copy and delete source
- file.copyTo(target, overwrite = true)
+ if (target.exists()) {
+ failed++
+ return@forEach
+ }
+ file.copyTo(target, overwrite = false)
if (file.delete()) {
moved++
} else {
// try overwrite move on legacy devices
// if (file.renameTo(target)) moved++ else failed++🤖 Prompt for AI Agents
In `@app/src/main/java/com/philkes/notallyx/utils/IOExtensions.kt` around lines
142 - 209, The migrateAllAttachments(ContextWrapper.migrateAllAttachments) logic
currently calls file.copyTo(target, overwrite = true) which contradicts the
intent in the comment to "skip copy and delete source" when a target exists and
risks clobbering data; change the routine to first check for target.exists() (or
compare file sizes/checksums if you want to handle conflicts) and if the target
exists skip copying and still delete the source (or skip delete if you prefer to
preserve both), otherwise perform a non-overwriting copy; also update the inline
comment to reflect the chosen behavior (skip or compare) so the code and comment
match.
Crustack
force-pushed
the
fix/attachments-public-folder
branch
from
93f7937 to
7a817c1
Compare
This was referenced Jan 25, 2026
This was referenced Mar 15, 2026
Merged
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 #784
dataInPublicFolderis enabled, all attachments are moved into the public/mediafolder of the appdataInPublicFolderis disabled, all attachments are moved into the private/datafolder of the appSummary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.