Open
Conversation
Allow users to upload an entire folder (with subfolders) into a vault, preserving the directory structure. Uses ACTION_OPEN_DOCUMENT_TREE for folder selection and recursively creates vault folders before uploading files. Resolves #63
|
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:
WalkthroughAdds folder-upload functionality across domain, presentation, persistence, and service layers. Domain: introduces StreamHelper, FileUploadedCallback, UploadFolderStructure, UploadFolderFiles, and updates UploadFiles; unit tests added. Presentation: UI elements and resources for folder selection, UploadFolderStructureBuilder, presenter updates, CryptomatorApp bindings, UploadNotification, UploadService foreground service, ResumeUploadDialog, and DI exposure for UploadCheckpointDao. Persistence: DB schema bumped to 14 with Upgrade13To14, UploadCheckpointEntity, and UploadCheckpointDao. Build/manifest updated for permissions and dependencies. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderStructure.java (1)
22-28: Mutable internal lists exposed via getters.
getFiles()andgetSubfolders()return the backingArrayListdirectly, so any caller can mutate the structure without going throughaddFile/addSubfolder. If this class is meant to be built incrementally and then consumed as read-only, consider returningCollections.unmodifiableList(...)from the getters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderStructure.java` around lines 22 - 28, The getters getFiles() and getSubfolders() expose the mutable backing lists directly; change them to return read-only views by wrapping the internal lists with Collections.unmodifiableList(...) (and add the java.util.Collections import) so callers cannot mutate the internal state; keep addFile/addSubfolder methods for controlled mutation and ensure the returned unmodifiable lists reflect the current contents without allowing external modification.domain/src/main/java/org/cryptomator/domain/usecases/cloud/StreamHelper.java (1)
16-27:closeQuietlyon the output stream may silently swallow flush/write errors.
copy()closes both streams viacloseQuietlyin thefinallyblock. Ifout.close()fails (e.g., aBufferedOutputStreamthat flushes on close, or a remote-backed stream), the data-loss error is silently swallowed and the caller believes the copy succeeded.Consider closing
outnormally (propagating the exception) and only usingcloseQuietlyforinand foroutin the error/finally-cleanup path.♻️ Proposed fix
static void copy(InputStream in, OutputStream out) throws IOException { byte[] buffer = new byte[BUFFER_SIZE]; try { int read; while ((read = in.read(buffer)) != EOF) { out.write(buffer, 0, read); } + out.close(); } finally { closeQuietly(in); closeQuietly(out); } }Calling
out.close()inside thetryis safe becausecloseQuietly(out)infinallyis a no-op on an already-closed stream. On the happy path any flush/write error inclose()now propagates; on the error path the stream is still cleaned up quietly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/src/main/java/org/cryptomator/domain/usecases/cloud/StreamHelper.java` around lines 16 - 27, The copy method currently closes both streams via closeQuietly in the finally block which can swallow flush/write exceptions from the OutputStream; modify StreamHelper.copy so that after the write loop completes you explicitly call out.close() inside the try block (so any flush/close exceptions propagate) and in the finally block still call closeQuietly(in) and closeQuietly(out) as a safety cleanup; update references to copy, closeQuietly, BUFFER_SIZE and EOF accordingly so the happy-path close propagates errors but resources are still quietly cleaned up on error.domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java (1)
85-131: Extract duplicated upload/copy logic into a shared utility class.
copyDataToFile()is identical in both classes (lines 119-131 in UploadFolderFiles, lines 98-110 in UploadFiles). Additionally, thecancelledFlagpattern,onCancel()method,execute()exception-wrapping, and most of theupload()andwriteCloudFile()implementations are duplicated across both classes. Extract these into utility methods (following the existingStreamHelperpattern) to reduce maintenance burden and inconsistency risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java` around lines 85 - 131, There is duplicated logic between UploadFolderFiles and UploadFiles (copyDataToFile, upload/writeCloudFile flows, cancelledFlag/onCancel pattern and execute exception wrapping); extract a shared utility (e.g., UploadUtils) containing: a copyDataToFile(DataSource, Context, cancelledFlag) that reproduces the existing behavior used by both classes, a writeCloudFile helper that wraps the cloudContentRepository.write call (taking CloudFolder/CloudFile, CancelAwareDataSource, size, replacing, ProgressAware), and shared helpers for creating CancelAwareDataSource and unified execute/exception-wrapping logic; update UploadFolderFiles and UploadFiles to call these new UploadUtils methods and remove the duplicated methods (preserve use of CancelAwareDataSource.wrap, dataSource.modifiedDate behavior, and deletion of temp file). Ensure method names referenced here (copyDataToFile, upload, writeCloudFile, CancelAwareDataSource, cancelledFlag, onCancel, execute) are replaced with calls into the shared utility.presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (1)
1092-1092:takePersistableUriPermissionis unnecessary for a one-time manual upload.The URI permission granted by
ACTION_OPEN_DOCUMENT_TREEis valid for the lifetime of the current process, which is sufficient for the asynchronous upload that follows. CallingtakePersistableUriPermissionstores an entry inContentResolver.getPersistedUriPermissions()that survives app restarts. Repeated use of folder upload (and every device reboot and reinstall cycle) will accumulate these stale entries until they hit the system cap.♻️ Remove persistent permission grant for one-time upload
- context().contentResolver.takePersistableUriPermission(treeUri, Intent.FLAG_GRANT_READ_URI_PERMISSION)If persistent access were needed in the future (e.g., for recurring uploads), it should be explicitly stored alongside its revocation path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt` at line 1092, Remove the unnecessary persistent grant: delete the call to contentResolver.takePersistableUriPermission(treeUri, Intent.FLAG_GRANT_READ_URI_PERMISSION) in BrowseFilesPresenter (the code that handles the ACTION_OPEN_DOCUMENT_TREE result). The one-time permission granted by the document picker is sufficient for the async upload, so simply remove that invocation (leave use of treeUri for the upload intact); if persistent access is ever required in future, obtain and store it explicitly along with a revocation path instead of blindly calling takePersistableUriPermission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java`:
- Around line 119-131: The copyDataToFile method currently creates a temp file
(created via createTempFile in copyDataToFile) and then opens streams
(CancelAwareDataSource.wrap(...).open(...) and new FileOutputStream(target)) but
if StreamHelper.copy or opening the stream throws, the temp file referenced by
target leaks; update copyDataToFile to use try-with-resources for the
InputStream and OutputStream and ensure the temp file (target) is deleted on any
exception (e.g., in a catch or finally block before rethrowing
FatalBackendException), while still preserving the setLastModified call when
copy succeeds; reference the method copyDataToFile, the target variable,
CancelAwareDataSource.wrap(...).open(...), StreamHelper.copy, createTempFile and
FatalBackendException when locating where to apply this change.
In `@presentation/build.gradle`:
- Line 188: The project is using an outdated DocumentFile dependency; update the
version constant androidxDocumentfileVersion in buildsystem/dependencies.gradle
from 1.0.1 to 1.1.0 so the dependency referenced by implementation
dependencies.documentFile (used for DocumentFile.fromTreeUri()) pulls 1.1.0;
after bumping the constant, re-sync the Gradle project to ensure implementation
dependencies.documentFile resolves to the new 1.1.0 artifact.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt`:
- Around line 1108-1129: The recursive buildUploadFolderStructure(DocumentFile)
can overflow the JVM stack for deeply nested directories; replace it with an
iterative implementation that uses an explicit stack/Deque of work items (e.g.,
pairs of DocumentFile and its corresponding UploadFolderStructure) to traverse
the tree: start by creating the root UploadFolderStructure(documentFile.name ?:
"folder"), push (documentFile, root) on the stack, then loop popping a
(currentDir, currentStruct), call currentDir.listFiles(), for each child push a
new (child, newSubStruct) for directories (and call
currentStruct.addSubfolder(newSubStruct)), and for files create UploadFile via
UploadFile.anUploadFile().withFileName(...).withDataSource(UriBasedDataSource.from(child.uri)).thatIsReplacing(false).build()
and add with currentStruct.addFile(...); this preserves existing symbols
(buildUploadFolderStructure, UploadFolderStructure, UploadFile,
UriBasedDataSource, DocumentFile) and avoids recursion/StackOverflowError.
- Around line 1115-1124: child.name can be null and currently silently skips
adding the file; update the block handling child.isFile (around
UploadFile.anUploadFile(), UriBasedDataSource.from(child.uri), and
structure.addFile) to handle null names explicitly: if child.name is null, emit
a warning via the presenter/logger noting the synthetic/unnamed entry (include
child.uri for context) and either supply a deterministic fallback name (e.g.,
derived from child.uri or a generated "unnamed-<id>" token) before building the
UploadFile, or add a clear code comment if dropping is intentional; also ensure
totalFileCount() accounting matches this behavior.
- Around line 1095-1106: Replace the raw Thread that calls
buildUploadFolderStructure(...) with a lifecycle-aware RxJava Single (or at
minimum wrap the background work in try/catch/finally) so exceptions can't kill
the thread and the progress spinner is always cleared: perform
buildUploadFolderStructure(documentFile) on Schedulers.io(), observe on
AndroidSchedulers.mainThread(), handle errors in onError to call
view?.showMessage(...) and always call
view?.showProgress(ProgressModel.COMPLETED) in the terminal path
(finally/DoFinally or Single.doFinally) before returning; ensure you no longer
call activity().runOnUiThread directly after the background work but use the
Android main scheduler (or check activity() is non-null) so callbacks aren't
executed against a destroyed Activity, and keep the existing
uploadFolder(folderStructure) call in the success path.
---
Nitpick comments:
In
`@domain/src/main/java/org/cryptomator/domain/usecases/cloud/StreamHelper.java`:
- Around line 16-27: The copy method currently closes both streams via
closeQuietly in the finally block which can swallow flush/write exceptions from
the OutputStream; modify StreamHelper.copy so that after the write loop
completes you explicitly call out.close() inside the try block (so any
flush/close exceptions propagate) and in the finally block still call
closeQuietly(in) and closeQuietly(out) as a safety cleanup; update references to
copy, closeQuietly, BUFFER_SIZE and EOF accordingly so the happy-path close
propagates errors but resources are still quietly cleaned up on error.
In
`@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java`:
- Around line 85-131: There is duplicated logic between UploadFolderFiles and
UploadFiles (copyDataToFile, upload/writeCloudFile flows, cancelledFlag/onCancel
pattern and execute exception wrapping); extract a shared utility (e.g.,
UploadUtils) containing: a copyDataToFile(DataSource, Context, cancelledFlag)
that reproduces the existing behavior used by both classes, a writeCloudFile
helper that wraps the cloudContentRepository.write call (taking
CloudFolder/CloudFile, CancelAwareDataSource, size, replacing, ProgressAware),
and shared helpers for creating CancelAwareDataSource and unified
execute/exception-wrapping logic; update UploadFolderFiles and UploadFiles to
call these new UploadUtils methods and remove the duplicated methods (preserve
use of CancelAwareDataSource.wrap, dataSource.modifiedDate behavior, and
deletion of temp file). Ensure method names referenced here (copyDataToFile,
upload, writeCloudFile, CancelAwareDataSource, cancelledFlag, onCancel, execute)
are replaced with calls into the shared utility.
In
`@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderStructure.java`:
- Around line 22-28: The getters getFiles() and getSubfolders() expose the
mutable backing lists directly; change them to return read-only views by
wrapping the internal lists with Collections.unmodifiableList(...) (and add the
java.util.Collections import) so callers cannot mutate the internal state; keep
addFile/addSubfolder methods for controlled mutation and ensure the returned
unmodifiable lists reflect the current contents without allowing external
modification.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt`:
- Line 1092: Remove the unnecessary persistent grant: delete the call to
contentResolver.takePersistableUriPermission(treeUri,
Intent.FLAG_GRANT_READ_URI_PERMISSION) in BrowseFilesPresenter (the code that
handles the ACTION_OPEN_DOCUMENT_TREE result). The one-time permission granted
by the document picker is sufficient for the async upload, so simply remove that
invocation (leave use of treeUri for the upload intact); if persistent access is
ever required in future, obtain and store it explicitly along with a revocation
path instead of blindly calling takePersistableUriPermission.
domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt
Outdated
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (15)
presentation/src/main/res/values/strings.xml (1)
630-640: Optional: add a unit label tonotification_upload_messagefor clarity
"Uploading %1$d/%2$d"leaves the numbers ambiguous in the notification shade (files? MB?). The analogousdialog_upload_file_remainingstring at line 406 uses"File %1$d of %2$d". A small label like"Uploading %1$d/%2$d files"would be more user-friendly, and is consistent with the finished/failed messages in the same block that all reference "files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/res/values/strings.xml` around lines 630 - 640, Update the notification string named notification_upload_message to include a unit label so the progress is unambiguous (e.g., change the text from "Uploading %1$d/%2$d" to "Uploading %1$d/%2$d files"); locate notification_upload_message in the strings.xml near the other upload strings and mirror the phrasing used by dialog_upload_file_remaining to keep consistency with finished/failed messages.data/src/main/java/org/cryptomator/data/db/entities/UploadCheckpointEntity.java (1)
33-37:@NotNullon primitive fieldsintandlongis redundant.GreenDAO's
@NotNullannotation onint/longprimitives is a no-op at the Java level (primitives can never be null). The generated SQLite column isNOT NULLfor primitives regardless. Removing the annotations reduces noise.♻️ Proposed cleanup
- `@NotNull` private int totalFileCount; - `@NotNull` private long timestamp;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/java/org/cryptomator/data/db/entities/UploadCheckpointEntity.java` around lines 33 - 37, Remove the redundant `@NotNull` annotations from the primitive fields in UploadCheckpointEntity: delete `@NotNull` on the fields totalFileCount (int) and timestamp (long) since primitives cannot be null and GreenDAO already generates NOT NULL columns for them; leave `@NotNull` on non-primitive fields if present and adjust any import usage if that annotation becomes unused.presentation/src/main/java/org/cryptomator/presentation/di/component/ApplicationComponent.java (1)
7-7:UploadCheckpointDao(data layer) exposed directly through the presentation DI component — consider a domain-level abstraction.All other providers in
ApplicationComponent(vaultRepository(),cloudContentRepository(), etc.) return domain-layer interfaces. Adding a concrete data-layer DAO here creates a direct presentation→data coupling that bypasses the domain boundary and makes the presentation layer harder to test in isolation.Consider introducing a
UploadCheckpointRepositoryinterface in thedomainmodule, implementing it in thedatamodule, and providing that interface through the component instead.Also applies to: 51-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/di/component/ApplicationComponent.java` at line 7, ApplicationComponent currently exposes the data-layer UploadCheckpointDao directly (symbol: UploadCheckpointDao) which leaks the data layer into presentation; add a domain-level abstraction UploadCheckpointRepository in the domain module, implement it in the data module by adapting UploadCheckpointDao, and change ApplicationComponent to provide UploadCheckpointRepository instead of UploadCheckpointDao (update provider method signatures and any injection sites to use UploadCheckpointRepository) so the presentation layer depends only on the domain interface.presentation/src/main/java/org/cryptomator/presentation/presenter/UploadFolderStructureBuilder.kt (1)
11-13: Unbounded recursion — consider adding an explicit depth limit or iterative traversal.
buildUploadFolderStructurecalls itself for every subdirectory without any depth guard. Deeply nested folder hierarchies (e.g., auto-generated project structures) can cause aStackOverflowError. An explicit max-depth constant and an early return (or a log + skip) would add a cheap safety net.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/presenter/UploadFolderStructureBuilder.kt` around lines 11 - 13, The recursive call in buildUploadFolderStructure has no depth guard and can overflow the stack; add a max-depth safety check (e.g., MAX_UPLOAD_DEPTH constant) and thread a currentDepth parameter (or convert to an iterative traversal) so that buildUploadFolderStructure(child) is only called when currentDepth < MAX_UPLOAD_DEPTH; when the limit is reached, skip adding deeper subfolders (or log a warning) instead of recursing, ensuring addSubfolder(...) only receives built structures within the allowed depth.domain/src/main/java/org/cryptomator/domain/usecases/cloud/FileUploadedCallback.java (1)
5-5: Consider clarifying therelativePathparameter contract.Per the AI summary,
UploadFilesinvokes this callback with just a plainfileName, not a relative path. The parameter namerelativePathimplies a path with separators, which will be incorrect for single-file upload callers. A neutral name (e.g.,fileIdentifier) or a Javadoc explaining the convention (plain name forUploadFiles, relative path forUploadFolderFiles) would prevent misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/src/main/java/org/cryptomator/domain/usecases/cloud/FileUploadedCallback.java` at line 5, The parameter name on FileUploadedCallback.onFileUploaded should be clarified: either rename the parameter from relativePath to a neutral identifier (e.g., fileIdentifier) across the FileUploadedCallback interface and all callers (including UploadFiles and UploadFolderFiles), or add Javadoc to onFileUploaded that states the exact contract (e.g., "plain file name when called from UploadFiles, relative path when called from UploadFolderFiles"); update the interface signature or its Javadoc and adjust/verify all implementations (and callers) such as UploadFiles and UploadFolderFiles to match the new name/contract.presentation/src/main/java/org/cryptomator/presentation/ui/dialog/ResumeUploadDialog.kt (2)
47-49: Unsafe castcontext as Callbackwill produce an unhelpfulClassCastException.If a caller passes a
Contextthat doesn't implementCallback, this crashes without a clear message. Consider adding a guard with a descriptive error.Proposed fix
init { - callback = context as Callback + require(context is Callback) { + "${context.javaClass.simpleName} must implement ResumeUploadDialog.Callback" + } + callback = context }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/ui/dialog/ResumeUploadDialog.kt` around lines 47 - 49, The init block in ResumeUploadDialog currently does an unsafe cast "callback = context as Callback" which can throw a ClassCastException with no clear message; change this to validate the context first (e.g., check "context is Callback") and either assign via a safe cast or throw an IllegalArgumentException with a descriptive message like "ResumeUploadDialog requires a Context implementing ResumeUploadDialog.Callback" so callers get a clear error; update the init block that sets the callback in the ResumeUploadDialog class accordingly.
22-27: Prefercontext.getString(resId, ...)overString.format(context.getString(resId), ...).
Context.getString(int, Object...)handles format arguments directly and is the idiomatic Android approach.Proposed fix
- .setMessage( - String.format( - context.getString(R.string.dialog_resume_upload_message), - completedCount, - totalCount - ) - ) + .setMessage( + context.getString(R.string.dialog_resume_upload_message, completedCount, totalCount) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/ui/dialog/ResumeUploadDialog.kt` around lines 22 - 27, Replace the manual String.format call in ResumeUploadDialog (where you call setMessage with String.format(context.getString(R.string.dialog_resume_upload_message), completedCount, totalCount)) with the idiomatic Context.getString overload: call context.getString(R.string.dialog_resume_upload_message, completedCount, totalCount) and pass that to setMessage; this removes the unnecessary String.format wrapper and uses the platform formatting API in the ResumeUploadDialog.kt code path.data/src/main/java/org/cryptomator/data/db/UploadCheckpointDao.java (1)
28-37:insertOrReplaceuses a read-then-write pattern instead of SQLite's native upsert.The
findByVaultId→ conditionalupdate/insertOrThrowpattern is a TOCTOU concern. While the unique index prevents duplicate rows, a concurrent call could cause an uncaughtSQLiteConstraintExceptionfrominsertOrThrow. Since the table already has a unique index onVAULT_ID, you can useinsertWithOnConflictwithCONFLICT_REPLACEto atomically upsert in a single statement.Proposed simplification
public long insertOrReplace(UploadCheckpointEntity entity) { ContentValues values = toContentValues(entity); - UploadCheckpointEntity existing = findByVaultId(entity.getVaultId()); - if (existing != null) { - getDb().update(TABLE_NAME, values, "VAULT_ID = ?", new String[]{existing.getVaultId().toString()}); - return existing.getId(); - } else { - return getDb().insertOrThrow(TABLE_NAME, null, values); - } + return getDb().insertWithOnConflict(TABLE_NAME, null, values, SQLiteDatabase.CONFLICT_REPLACE); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/java/org/cryptomator/data/db/UploadCheckpointDao.java` around lines 28 - 37, Replace the current read-then-write implementation in insertOrReplace(UploadCheckpointEntity) (which calls findByVaultId and conditionally update/insertOrThrow) with a single atomic upsert using getDb().insertWithOnConflict(TABLE_NAME, null, toContentValues(entity), SQLiteDatabase.CONFLICT_REPLACE) so concurrent inserts won’t throw a SQLiteConstraintException; ensure you remove the conditional branch and return the long row id returned by insertWithOnConflict.presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt (1)
1100-1117:onFileUploadCompleted()is called synchronously after dispatching an async upload.Both
uploadFolder(line 1115) anduploadFiles(line 434) callonFileUploadCompleted()immediately aftercryptomatorApp.startFolderUpload/startFileUpload. This clearsuploadLocationand showsProgressModel.COMPLETEDbefore the background service has done any work. If the design intent is "fire and forget to the service," this is fine — but the method nameonFileUploadCompletedis misleading since the upload hasn't completed yet. Consider renaming to clarify intent (e.g.,onUploadDispatched).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt` around lines 1100 - 1117, The call to onFileUploadCompleted() is misleading because uploadFolder and uploadFiles call it immediately after dispatching async work via cryptomatorApp.startFolderUpload and cryptomatorApp.startFileUpload; either rename onFileUploadCompleted to reflect dispatch semantics (e.g., onUploadDispatched) and update all usages, or remove the immediate call and instead invoke the real completion handler when the background service signals completion (hook into the service's callback/event where startFolderUpload/startFileUpload reports results); update references in BrowseFilesPresenter (uploadFolder, uploadFiles) and any observers of onFileUploadCompleted accordingly.presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt (2)
566-590:onResumeUploadConfirmedperforms potentially blocking DB and repository calls on the UI thread.
dao.findByVaultId,applicationComponent().vaultRepository().load(), andapplicationComponent().cloudRepository().decryptedViewOf()are all synchronous calls executed on the calling (main) thread. If the vault repository or cloud repository accesses involve I/O (network, crypto, or database), this will block the UI.Consider wrapping this logic in a background use-case or coroutine, consistent with how other vault-loading flows in this presenter use
DefaultResultHandlercallbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt` around lines 566 - 590, onResumeUploadConfirmed is doing blocking work on the UI thread (dao.findByVaultId, applicationComponent().vaultRepository().load, applicationComponent().cloudRepository().decryptedViewOf); move those synchronous calls into a background coroutine or existing use-case pattern (e.g., DefaultResultHandler) so I/O happens off the main thread, then dispatch back to the main thread to call handleFolderResume/handleFileResume and navigatePendingAfterResume; ensure errors are caught and handled on the background task (preserving the Timber.error + dao.deleteByVaultId behavior) and that any UI interactions occur on the main dispatcher.
540-561:checkForInterruptedUploadperforms a synchronous DAO query on the main thread.
dao.findByVaultId(vault.id)at Line 552 is called from theonSuccesscallback of a use-case result handler, which runs on the main thread. Database access here may cause UI jank or ANR on slower devices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt` around lines 540 - 561, checkForInterruptedUpload currently calls dao.findByVaultId(vault.id) on the main thread (called from the onSuccess callback), which can block UI; change it to perform the DB lookup off the main thread (e.g. use an Executor/IO coroutine Dispatcher) and then resume on the main thread to update UI: run findByVaultId and parseJsonArraySize on a background thread, then on the main thread call ResumeUploadDialog.withContext(...).show(...) and set pendingNavigationAfterResume or call navigateToVaultContent(vault, folder) as appropriate; update checkForInterruptedUpload to accept/handle the async result and ensure any UI interactions (ResumeUploadDialog.withContext, pendingNavigationAfterResume, navigateToVaultContent) happen on the main thread.domain/src/test/java/org/cryptomator/domain/usecases/cloud/UploadFolderFilesTest.kt (1)
425-458: Test utility methodsdataSourceWithBytesandbytesare duplicated fromUploadFileTest.kt.These helper methods are identical to those in
UploadFileTest.kt(per the relevant code snippet at lines 95-121). Consider extracting them into a shared test utility to reduce duplication across upload test classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/src/test/java/org/cryptomator/domain/usecases/cloud/UploadFolderFilesTest.kt` around lines 425 - 458, The methods dataSourceWithBytes(...) and bytes(...) duplicated from UploadFileTest.kt should be extracted into a shared test utility to remove duplication: create a new test helper class (e.g., TestDataSources or UploadTestUtils) containing these two functions (preserving their signatures and behavior), replace the local implementations in UploadFolderFilesTest (and UploadFileTest) with calls/imports to that utility, and update tests to import/use the shared functions instead of defining them locally.presentation/src/main/java/org/cryptomator/presentation/service/UploadNotification.kt (1)
132-138: Notification channel name is hardcoded in English.
NOTIFICATION_CHANNEL_NAMEon Line 136 is a hardcoded English string. It should reference a string resource for proper localization, consistent with how other strings in this class usecontext.getString(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/service/UploadNotification.kt` around lines 132 - 138, The NOTIFICATION_CHANNEL_NAME constant in the companion object is hardcoded in English; change the code to obtain the channel name from a string resource instead of a compile-time constant: add a string resource (e.g. R.string.notification_channel_cryptomator_upload) in strings.xml, remove or replace NOTIFICATION_CHANNEL_NAME so the UploadNotification code that creates the NotificationChannel uses context.getString(R.string.notification_channel_cryptomator_upload) (similar to other uses of context.getString in this class), and update any references to NOTIFICATION_CHANNEL_NAME to use the context-derived string.domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java (1)
29-164: Extract duplicate upload mechanics into a shared base class or utility.Substantial code duplication exists between
UploadFiles.javaandUploadFolderFiles.java: the cancel flag,execute()method with cancellation wrapping,copyDataToFile(), and theupload()overloads are either identical or differ only in parameter threading. ThewriteCloudFile()method also follows the same pattern, with the only difference being whether aCloudFolderparameter is passed or theparentfield is used.Consider extracting these shared mechanics into a common base class or utility helper, keeping only the folder-creation and recursive traversal logic unique to
UploadFolderFiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java` around lines 29 - 164, There is duplicated upload/cancellation logic between UploadFolderFiles and UploadFiles; extract the shared mechanics (fields: cancelled, cancelledFlag/Flag, fileUploadedCallback if shared; methods: execute(), copyDataToFile(), both upload(...) overloads, writeCloudFile(...), and usage of CancelAwareDataSource.wrap and FileBasedDataSource.from) into a common base class or utility (e.g., AbstractUpload or UploadHelper) that exposes a protected method to perform the low-level write (writeCloudFile) and a protected API to supply folder-specific behavior (e.g., createFolderSafe/recursive traversal in UploadFolderFiles); update UploadFolderFiles to extend/use that base and remove duplicated methods so it only implements folder-creation and traversal (createFolderSafe, uploadFolder) while delegating to the common implementation for actual file copying/writing and cancellation handling.presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java (1)
213-239: Remove the unusedcontentResolverUtilfield or expose it through a binder getter.The field at line 42 is initialized in
Binder.init()(line 221) but never referenced elsewhere inUploadService. UnlikeuploadCheckpointDao, which has a getter method,contentResolverUtilis not exposed to callers. Either remove it if it's not needed, or add a getter method if it's intended for use by clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java` around lines 213 - 239, The UploadService class contains an initialized but unused field contentResolverUtil (set in Binder.init()), so either remove the UploadService.contentResolverUtil field and its initialization in Binder.init() if it’s not needed, or add a public accessor in UploadService (e.g., getContentResolverUtil()) and expose it via Binder by adding a corresponding getContentResolverUtil() method that returns UploadService.this.contentResolverUtil; update Binder.init() only if keeping the field to ensure it remains initialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/src/androidTest/java/org/cryptomator/data/db/UpgradeDatabaseTest.kt`:
- Around line 994-1007: The catch currently swallows any Exception after the
duplicate insert, so replace the broad catch with an assertion that the thrown
exception is the expected constraint type: catch specifically for
android.database.sqlite.SQLiteConstraintException (or at minimum
android.database.SQLException) or catch Exception and
assertThat(e).isInstanceOf(SQLiteConstraintException::class.java), referencing
the test block that performs Sql.insertInto("UPLOAD_CHECKPOINT_ENTITY") and the
surrounding try/catch in UpgradeDatabaseTest to ensure only unique-constraint
failures are treated as expected.
In
`@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java`:
- Around line 107-114: createFolderSafe currently returns a locally constructed
CloudFolder reference on CloudNodeAlreadyExistsException without validating it
against the server; update the catch block in createFolderSafe to resolve or
verify the folder server-side (e.g., call
cloudContentRepository.resolve(cloudContentRepository.folder(parent,
folderName)) or use an exists() check) and return the resolved/validated
CloudFolder so both success and exception paths perform the same server-side
validation.
In `@presentation/src/main/AndroidManifest.xml`:
- Around line 215-218: Add an explicit android:exported="false" attribute to the
service declarations for .service.UploadService (and the similar
.service.AutoUploadService) in the manifest so the services are explicitly
non-exported; locate the <service android:name=".service.UploadService" ... />
and the <service android:name=".service.AutoUploadService" ... /> entries and
add android:exported="false" alongside the existing attributes.
In `@presentation/src/main/java/org/cryptomator/presentation/CryptomatorApp.kt`:
- Around line 218-220: The current toJsonArray function correctly escapes
strings, but VaultListPresenter.parseJsonArray uses a fragile split(",") which
breaks on values containing commas; replace the manual splitting in
VaultListPresenter.parseJsonArray with a robust JSON parser (e.g., construct an
org.json.JSONArray from the stored string and iterate to extract each element)
so both sides use proper JSON array serialization/deserialization and handle
escaped quotes/commas consistently with toJsonArray.
- Around line 187-195: startFileUpload and startFolderUpload currently no-op
when uploadServiceBinder is null, silently dropping user requests; update both
methods (startFileUpload, startFolderUpload) to check uploadServiceBinder and
when null log a warning (use the class/process logger) and surface user-visible
feedback (e.g., show a Toast or a UI notification / dialog or invoke an error
callback) so the user knows the upload wasn't started, while keeping the
existing call path when uploadServiceBinder is present.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/UploadFolderStructureBuilder.kt`:
- Around line 14-23: When building file entries in UploadFolderStructureBuilder
(inside the child.isFile branch where child.name is checked), don't silently
drop files with null names — log a warning including the document URI and skip
adding the file. Modify the child.name?.let { ... } path to add an else branch
that calls the presenter’s logger (e.g., logger.warn or similar) with a message
like "Skipping file with null name" and the child.uri, then return/skip; leave
the existing structure.addFile/UploadFile.anUploadFile/UriBasedDataSource.from
logic unchanged for the non-null case.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt`:
- Around line 601-613: The catch block for SecurityException in
VaultListPresenter around the DocumentFile.fromTreeUri(...) /
buildUploadFolderStructure / cryptomatorApp.startFolderUpload flow currently
swallows the exception; update that catch to log the caught SecurityException
(including the exception object) using the presenter’s existing logging facility
(or android.util.Log if none) before falling through to view?.showMessage(...)
and cryptomatorApp.getUploadCheckpointDao()?.deleteByVaultId(vaultId); ensure
the log entry includes context (e.g., "resume upload: SecurityException while
accessing tree uri") and the exception so diagnostics preserve stacktrace.
- Around line 655-660: The current parseJsonArray function hand-parses JSON and
breaks on values with commas or quotes; replace its implementation to use
org.json.JSONArray for robust parsing: in parseJsonArray(json: String?) (used
for pendingFileUris), return an empty list for null/empty inputs, construct a
JSONArray(json), iterate its elements and collect each element.toString() into a
List<String>, and handle JSONException by returning emptyList() or propagating
as appropriate; this ensures URIs containing commas/quotes are parsed correctly.
In
`@presentation/src/main/java/org/cryptomator/presentation/service/UploadNotification.kt`:
- Around line 64-81: The notification shows an off-by-one when the last file
finishes because update() always displays completedFiles + 1 and
updateFinishedFile() increments completedFiles before calling update(); fix by
clamping the displayed index inside update() (e.g., compute displayIndex =
min(completedFiles + 1, totalFiles)) and use that displayIndex in the format
call so the builder in update() never shows a number greater than totalFiles;
keep updateFinishedFile() as-is (it still increments completedFiles) so the
clamped logic in update() prevents "6 of 5" glitches.
In
`@presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java`:
- Around line 149-159: The throttling fields startTimeNotificationDelay and
elapsedTimeNotificationDelay used in updateNotification are accessed from both
the worker thread and the Handler.post lambda on the main thread without
synchronization; make these fields volatile (or move the entire throttling logic
into the main-thread Runnable) so reads/writes are always visible across threads
and eliminate the race condition in updateNotification.
- Around line 57-66: There is a race where cancelCallback is assigned inside the
worker lambda (in startFileUpload/startFolderUpload via
UploadFiles/UploadFolderFiles) after worker.start(), so a cancel that arrives
earlier never propagates; fix by assigning cancelCallback before calling execute
and immediately invoking it if the service-level cancelled flag is already true:
in startFileUpload and the analogous startFolderUpload, set cancelCallback =
uploadFiles::onCancel (or uploadFolderFiles::onCancel) prior to calling
uploadFiles.execute(progressAware) (and after constructing the upload object),
then if (cancelled) call cancelCallback.run()/onCancel() to propagate any prior
cancel; also ensure onStartCommand still only toggles the cancelled flag and
calls cancelCallback if non-null.
- Around line 79-117: startUpload currently overwrites shared mutable fields
(worker, notification, cancelCallback, cancelled) unconditionally so concurrent
calls corrupt state; add a guard at the start of startUpload that checks the
existing worker (e.g., if worker != null && worker.isAlive()) and either rejects
the new upload or enqueues it, and protect mutations of worker, notification,
cancelCallback and cancelled with a short synchronized block (or use an
AtomicReference for worker) so only one thread can start and set those fields;
also ensure the finally block in the worker clears/resets these shared fields
(worker = null, notification = null, cancelCallback = null, cancelled = false)
so subsequent uploads can proceed.
In `@presentation/src/main/res/values/strings.xml`:
- Line 635: The string resource notification_upload_finished_message uses a
singular/plural-sensitive count but is defined as a plain <string>, producing "1
files uploaded to vault"; replace it with a <plurals
name="notification_upload_finished_message"> providing quantity entries for
"one" (e.g., "1 file uploaded to vault") and "other" (e.g., "%d files uploaded
to vault"), and update any callers to fetch it via
getResources().getQuantityString(R.plurals.notification_upload_finished_message,
count, count) or the Kotlin equivalent so the correct form is chosen at runtime.
- Line 644: The string resource dialog_resume_upload_message uses a
plural-dependent phrase ("%1$d of %2$d files completed") that renders
incorrectly for singular counts; update this resource by either rewording to a
neutral form (e.g., "%1$d of %2$d completed" or "Upload interrupted (%1$d of
%2$d items completed). Resume?") or replace the inner "files completed" fragment
with a <plurals> resource and reference it so singular/plural forms are handled
correctly; locate the string by its resource name dialog_resume_upload_message
and implement the chosen change in strings.xml and, if using plurals, add the
corresponding <plurals> entry and use getQuantityString where the resource is
consumed.
---
Duplicate comments:
In
`@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java`:
- Around line 150-162: The copyDataToFile method leaks the temporary File
created by createTempFile("upload","tmp",dir) when opening streams or
StreamHelper.copy(...) throws; modify copyDataToFile to use try-with-resources
for the InputStream (from CancelAwareDataSource.wrap(...).open(context)) and
OutputStream (new FileOutputStream(target)) and ensure that if any exception
occurs after creating target you delete target (target.delete() or
Files.deleteIfExists) before rethrowing the FatalBackendException; keep the
modifiedDate handling (dataSource.modifiedDate(...).ifPresent(...)) after a
successful copy so the temp file is only touched when the copy completed.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt`:
- Around line 1087-1098: The background Thread that calls
buildUploadFolderStructure can throw and never reach
view?.showProgress(ProgressModel.COMPLETED) or call uploadFolder, and
activity().runOnUiThread is not lifecycle-safe; wrap the background work in
try/catch/finally, catch relevant exceptions (e.g.,
SecurityException/Exception), and in finally always post back to the UI to call
view?.showProgress(ProgressModel.COMPLETED) (and show an error via
view?.showMessage when appropriate), and use a lifecycle-safe UI post (e.g.,
check activity() is non-null and use activity()?.runOnUiThread or a safe
dispatcher) before calling uploadFolder(folderStructure, treeUri) so the UI
update and upload invocation always happen safely even on errors or destroyed
activity.
---
Nitpick comments:
In
`@data/src/main/java/org/cryptomator/data/db/entities/UploadCheckpointEntity.java`:
- Around line 33-37: Remove the redundant `@NotNull` annotations from the
primitive fields in UploadCheckpointEntity: delete `@NotNull` on the fields
totalFileCount (int) and timestamp (long) since primitives cannot be null and
GreenDAO already generates NOT NULL columns for them; leave `@NotNull` on
non-primitive fields if present and adjust any import usage if that annotation
becomes unused.
In `@data/src/main/java/org/cryptomator/data/db/UploadCheckpointDao.java`:
- Around line 28-37: Replace the current read-then-write implementation in
insertOrReplace(UploadCheckpointEntity) (which calls findByVaultId and
conditionally update/insertOrThrow) with a single atomic upsert using
getDb().insertWithOnConflict(TABLE_NAME, null, toContentValues(entity),
SQLiteDatabase.CONFLICT_REPLACE) so concurrent inserts won’t throw a
SQLiteConstraintException; ensure you remove the conditional branch and return
the long row id returned by insertWithOnConflict.
In
`@domain/src/main/java/org/cryptomator/domain/usecases/cloud/FileUploadedCallback.java`:
- Line 5: The parameter name on FileUploadedCallback.onFileUploaded should be
clarified: either rename the parameter from relativePath to a neutral identifier
(e.g., fileIdentifier) across the FileUploadedCallback interface and all callers
(including UploadFiles and UploadFolderFiles), or add Javadoc to onFileUploaded
that states the exact contract (e.g., "plain file name when called from
UploadFiles, relative path when called from UploadFolderFiles"); update the
interface signature or its Javadoc and adjust/verify all implementations (and
callers) such as UploadFiles and UploadFolderFiles to match the new
name/contract.
In
`@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java`:
- Around line 29-164: There is duplicated upload/cancellation logic between
UploadFolderFiles and UploadFiles; extract the shared mechanics (fields:
cancelled, cancelledFlag/Flag, fileUploadedCallback if shared; methods:
execute(), copyDataToFile(), both upload(...) overloads, writeCloudFile(...),
and usage of CancelAwareDataSource.wrap and FileBasedDataSource.from) into a
common base class or utility (e.g., AbstractUpload or UploadHelper) that exposes
a protected method to perform the low-level write (writeCloudFile) and a
protected API to supply folder-specific behavior (e.g.,
createFolderSafe/recursive traversal in UploadFolderFiles); update
UploadFolderFiles to extend/use that base and remove duplicated methods so it
only implements folder-creation and traversal (createFolderSafe, uploadFolder)
while delegating to the common implementation for actual file copying/writing
and cancellation handling.
In
`@domain/src/test/java/org/cryptomator/domain/usecases/cloud/UploadFolderFilesTest.kt`:
- Around line 425-458: The methods dataSourceWithBytes(...) and bytes(...)
duplicated from UploadFileTest.kt should be extracted into a shared test utility
to remove duplication: create a new test helper class (e.g., TestDataSources or
UploadTestUtils) containing these two functions (preserving their signatures and
behavior), replace the local implementations in UploadFolderFilesTest (and
UploadFileTest) with calls/imports to that utility, and update tests to
import/use the shared functions instead of defining them locally.
In
`@presentation/src/main/java/org/cryptomator/presentation/di/component/ApplicationComponent.java`:
- Line 7: ApplicationComponent currently exposes the data-layer
UploadCheckpointDao directly (symbol: UploadCheckpointDao) which leaks the data
layer into presentation; add a domain-level abstraction
UploadCheckpointRepository in the domain module, implement it in the data module
by adapting UploadCheckpointDao, and change ApplicationComponent to provide
UploadCheckpointRepository instead of UploadCheckpointDao (update provider
method signatures and any injection sites to use UploadCheckpointRepository) so
the presentation layer depends only on the domain interface.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt`:
- Around line 1100-1117: The call to onFileUploadCompleted() is misleading
because uploadFolder and uploadFiles call it immediately after dispatching async
work via cryptomatorApp.startFolderUpload and cryptomatorApp.startFileUpload;
either rename onFileUploadCompleted to reflect dispatch semantics (e.g.,
onUploadDispatched) and update all usages, or remove the immediate call and
instead invoke the real completion handler when the background service signals
completion (hook into the service's callback/event where
startFolderUpload/startFileUpload reports results); update references in
BrowseFilesPresenter (uploadFolder, uploadFiles) and any observers of
onFileUploadCompleted accordingly.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/UploadFolderStructureBuilder.kt`:
- Around line 11-13: The recursive call in buildUploadFolderStructure has no
depth guard and can overflow the stack; add a max-depth safety check (e.g.,
MAX_UPLOAD_DEPTH constant) and thread a currentDepth parameter (or convert to an
iterative traversal) so that buildUploadFolderStructure(child) is only called
when currentDepth < MAX_UPLOAD_DEPTH; when the limit is reached, skip adding
deeper subfolders (or log a warning) instead of recursing, ensuring
addSubfolder(...) only receives built structures within the allowed depth.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt`:
- Around line 566-590: onResumeUploadConfirmed is doing blocking work on the UI
thread (dao.findByVaultId, applicationComponent().vaultRepository().load,
applicationComponent().cloudRepository().decryptedViewOf); move those
synchronous calls into a background coroutine or existing use-case pattern
(e.g., DefaultResultHandler) so I/O happens off the main thread, then dispatch
back to the main thread to call handleFolderResume/handleFileResume and
navigatePendingAfterResume; ensure errors are caught and handled on the
background task (preserving the Timber.error + dao.deleteByVaultId behavior) and
that any UI interactions occur on the main dispatcher.
- Around line 540-561: checkForInterruptedUpload currently calls
dao.findByVaultId(vault.id) on the main thread (called from the onSuccess
callback), which can block UI; change it to perform the DB lookup off the main
thread (e.g. use an Executor/IO coroutine Dispatcher) and then resume on the
main thread to update UI: run findByVaultId and parseJsonArraySize on a
background thread, then on the main thread call
ResumeUploadDialog.withContext(...).show(...) and set
pendingNavigationAfterResume or call navigateToVaultContent(vault, folder) as
appropriate; update checkForInterruptedUpload to accept/handle the async result
and ensure any UI interactions (ResumeUploadDialog.withContext,
pendingNavigationAfterResume, navigateToVaultContent) happen on the main thread.
In
`@presentation/src/main/java/org/cryptomator/presentation/service/UploadNotification.kt`:
- Around line 132-138: The NOTIFICATION_CHANNEL_NAME constant in the companion
object is hardcoded in English; change the code to obtain the channel name from
a string resource instead of a compile-time constant: add a string resource
(e.g. R.string.notification_channel_cryptomator_upload) in strings.xml, remove
or replace NOTIFICATION_CHANNEL_NAME so the UploadNotification code that creates
the NotificationChannel uses
context.getString(R.string.notification_channel_cryptomator_upload) (similar to
other uses of context.getString in this class), and update any references to
NOTIFICATION_CHANNEL_NAME to use the context-derived string.
In
`@presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java`:
- Around line 213-239: The UploadService class contains an initialized but
unused field contentResolverUtil (set in Binder.init()), so either remove the
UploadService.contentResolverUtil field and its initialization in Binder.init()
if it’s not needed, or add a public accessor in UploadService (e.g.,
getContentResolverUtil()) and expose it via Binder by adding a corresponding
getContentResolverUtil() method that returns
UploadService.this.contentResolverUtil; update Binder.init() only if keeping the
field to ensure it remains initialized.
In
`@presentation/src/main/java/org/cryptomator/presentation/ui/dialog/ResumeUploadDialog.kt`:
- Around line 47-49: The init block in ResumeUploadDialog currently does an
unsafe cast "callback = context as Callback" which can throw a
ClassCastException with no clear message; change this to validate the context
first (e.g., check "context is Callback") and either assign via a safe cast or
throw an IllegalArgumentException with a descriptive message like
"ResumeUploadDialog requires a Context implementing ResumeUploadDialog.Callback"
so callers get a clear error; update the init block that sets the callback in
the ResumeUploadDialog class accordingly.
- Around line 22-27: Replace the manual String.format call in ResumeUploadDialog
(where you call setMessage with
String.format(context.getString(R.string.dialog_resume_upload_message),
completedCount, totalCount)) with the idiomatic Context.getString overload: call
context.getString(R.string.dialog_resume_upload_message, completedCount,
totalCount) and pass that to setMessage; this removes the unnecessary
String.format wrapper and uses the platform formatting API in the
ResumeUploadDialog.kt code path.
In `@presentation/src/main/res/values/strings.xml`:
- Around line 630-640: Update the notification string named
notification_upload_message to include a unit label so the progress is
unambiguous (e.g., change the text from "Uploading %1$d/%2$d" to "Uploading
%1$d/%2$d files"); locate notification_upload_message in the strings.xml near
the other upload strings and mirror the phrasing used by
dialog_upload_file_remaining to keep consistency with finished/failed messages.
data/src/androidTest/java/org/cryptomator/data/db/UpgradeDatabaseTest.kt
Show resolved
Hide resolved
domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/CryptomatorApp.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/CryptomatorApp.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/service/UploadNotification.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java (1)
99-128: Worker thread fields not cleared infinally— staleworker.isAlive()check on terminated thread.After the worker thread exits,
workerstill references the completedThreadobject. WhileisAlive()returnsfalsefor a terminated thread so the guard at line 87 still works, settingworker = nullinfinallymakes the intent clearer and avoids holding a reference to a dead thread.Proposed fix
} finally { + cancelCallback = null; + worker = null; stopForeground(STOP_FOREGROUND_DETACH); stopSelf(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java` around lines 99 - 128, The worker Thread field is left referencing a terminated Thread, which can be misleading and keeps a stale object; in the finally block of the run lambda inside UploadService (the code that calls stopForeground(STOP_FOREGROUND_DETACH) and stopSelf()), set the UploadService.worker field to null so the terminated Thread is cleared (ensure this assignment happens after any final cleanup so existing isAlive() checks continue to behave correctly).presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt (1)
568-592: DB and repository calls on main thread inonResumeUploadConfirmed.Lines 571 (
dao.findByVaultId), 574 (vaultRepository().load), and 582 (cloudRepository().decryptedViewOf) all perform I/O on the main thread. While these are likely fast for single-record lookups, they still risk ANR under contention. Consider wrapping the heavy lifting in a background thread if this becomes a problem in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt` around lines 568 - 592, onResumeUploadConfirmed is doing blocking I/O on the main thread (dao.findByVaultId, applicationComponent().vaultRepository().load, applicationComponent().cloudRepository().decryptedViewOf); move those calls to a background dispatcher (e.g., launch a coroutine on Dispatchers.IO) and then switch back to the main thread only for UI work (calling handleFolderResume/handleFileResume and navigatePendingAfterResume). Specifically, perform dao.findByVaultId, vaultRepository().load and cloudRepository().decryptedViewOf inside the IO block, capture results/exceptions there (preserve the Timber.e and dao.deleteByVaultId behavior on error), and then resume on the main dispatcher to call handleFolderResume or handleFileResume and navigatePendingAfterResume.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/src/androidTest/java/org/cryptomator/data/db/UpgradeDatabaseTest.kt`:
- Line 945: The test in UpgradeDatabaseTest.kt inserts a column value for
"PENDING_FILE_URIS" (insert .text("PENDING_FILE_URIS", null)) but the subsequent
assertion block (the expectation checks around the same test that validate the
upgraded schema/row) does not assert that column, leaving schema changes
undetected; update the assertion block in the same test to include an assertion
for the PENDING_FILE_URIS column (expecting null or the intended default) so the
test explicitly verifies the presence and value of that column after upgrade.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt`:
- Around line 657-665: The JSONException in parseJsonArray is currently
swallowed; change the catch block to log the exception for diagnostics before
returning emptyList(). Locate the parseJsonArray function and in the catch (e:
JSONException) block call the class's logger (e.g., logger.error or LOG.error)
or an appropriate logging facility to record a clear message like "Failed to
parse JSON array for checkpoints" and include the exception e, then return
emptyList().
- Around line 594-615: handleFolderResume is calling
buildUploadFolderStructure(documentFile) on the main thread which can block UI;
move the folder traversal off the UI thread (e.g., dispatch
buildUploadFolderStructure and subsequent startFolderUpload to a background
thread/Coroutine/Executor) and invoke view updates and navigation only from the
main thread when the background work completes. Specifically: in
handleFolderResume, instead of calling buildUploadFolderStructure synchronously,
submit a background task that calls buildUploadFolderStructure(documentFile) and
then calls cryptomatorApp.startFolderUpload(...) on completion; ensure any UI
calls (view?.showMessage(...) and navigatePendingAfterResume() from
onResumeUploadConfirmed) are executed only after the background task finishes
(e.g., by invoking navigatePendingAfterResume in the background task's success
callback on the UI thread) and preserve the existing SecurityException handling
and checkpoint deletion flow.
In
`@presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java`:
- Around line 154-158: The manual JSON serialization in toJsonArray(Set<String>
items) is fragile (only escapes quotes); replace it with org.json.JSONArray
usage: create a new JSONArray, iterate the items Set (or pass a Collection) and
put each string into the JSONArray so JSON escaping is handled correctly, then
return the JSONArray.toString(); update the toJsonArray method to use JSONArray
to produce the array string so it matches VaultListPresenter.parseJsonArray's
parsing.
- Around line 160-170: The first progress update is skipped because
elapsedTimeNotificationDelay starts at 0 so updateNotification() takes the else
branch; fix by initializing the delay state so the first call posts
immediately—set elapsedTimeNotificationDelay to a value >200 (e.g.
Long.MAX_VALUE or 201) or initialize startTimeNotificationDelay to
System.currentTimeMillis() at construction so the first call into
updateNotification() (method updateNotification) enters the post branch and
calls notification.update(asPercentage); update usages of
startTimeNotificationDelay and elapsedTimeNotificationDelay accordingly.
---
Duplicate comments:
In `@data/src/androidTest/java/org/cryptomator/data/db/UpgradeDatabaseTest.kt`:
- Around line 1005-1007: Detekt flags the empty catch of
android.database.sqlite.SQLiteConstraintException in UpgradeDatabaseTest as a
swallowed exception, but the empty body is intentional for this
constraint-violation test; suppress the warning by adding a
`@Suppress`("SwallowedException") annotation to the test method (or the
UpgradeDatabaseTest class) that contains the catch, keeping the catch as-is for
the assertion semantics.
In
`@domain/src/main/java/org/cryptomator/domain/usecases/cloud/UploadFolderFiles.java`:
- Around line 107-114: createFolderSafe currently returns a locally-constructed
CloudFolder reference in the catch block which lacks server-side validation;
change the catch for CloudNodeAlreadyExistsException in createFolderSafe to
retrieve and return the actual server-backed node from cloudContentRepository
(e.g. call the repository's lookup method such as
cloudContentRepository.find(...) or cloudContentRepository.getByPath(...) for
parent and folderName) instead of returning cloudContentRepository.folder(...),
and if that lookup fails throw a BackendException so callers always receive a
validated server-backed CloudFolder.
In `@presentation/src/main/res/values/strings.xml`:
- Line 647: The string resource dialog_resume_upload_message currently embeds a
plural-sensitive fragment "(%1$d of %2$d files completed)" which renders
awkwardly for singular counts; update the resource identified by name
"dialog_resume_upload_message" to use a plural-safe phrasing (for example
"%1$d/%2$d completed" or use Android plurals with <plurals> and reference the
correct quantity for the first parameter) so the displayed text is grammatically
correct for both singular and plural counts.
---
Nitpick comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt`:
- Around line 568-592: onResumeUploadConfirmed is doing blocking I/O on the main
thread (dao.findByVaultId, applicationComponent().vaultRepository().load,
applicationComponent().cloudRepository().decryptedViewOf); move those calls to a
background dispatcher (e.g., launch a coroutine on Dispatchers.IO) and then
switch back to the main thread only for UI work (calling
handleFolderResume/handleFileResume and navigatePendingAfterResume).
Specifically, perform dao.findByVaultId, vaultRepository().load and
cloudRepository().decryptedViewOf inside the IO block, capture
results/exceptions there (preserve the Timber.e and dao.deleteByVaultId behavior
on error), and then resume on the main dispatcher to call handleFolderResume or
handleFileResume and navigatePendingAfterResume.
In
`@presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java`:
- Around line 99-128: The worker Thread field is left referencing a terminated
Thread, which can be misleading and keeps a stale object; in the finally block
of the run lambda inside UploadService (the code that calls
stopForeground(STOP_FOREGROUND_DETACH) and stopSelf()), set the
UploadService.worker field to null so the terminated Thread is cleared (ensure
this assignment happens after any final cleanup so existing isAlive() checks
continue to behave correctly).
data/src/androidTest/java/org/cryptomator/data/db/UpgradeDatabaseTest.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt
Outdated
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
presentation/src/main/java/org/cryptomator/presentation/CryptomatorApp.kt (1)
206-221:createUploadCheckpointsilently no-ops when binder is null — upload proceeds without checkpoint protection.If
uploadServiceBinderis null at Line 208, the method returns without logging or feedback. The upload will still start (viastartFileUpload/startFolderUploadwhich log the drop), but if only the checkpoint fails while the binder becomes available moments later for the actual upload, files could be uploaded without any resume capability.Add a log statement consistent with the pattern used in the upload methods:
Proposed fix
fun createUploadCheckpoint(vaultId: Long, type: String, targetFolderPath: String, sourceFolderUri: String?, sourceFolderName: String?, pendingFileUris: List<String>, totalFileCount: Int) { - val dao = uploadServiceBinder?.uploadCheckpointDao ?: return + val dao = uploadServiceBinder?.uploadCheckpointDao ?: run { + Timber.tag("App").e("Upload service not connected, checkpoint creation skipped") + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/CryptomatorApp.kt` around lines 206 - 221, createUploadCheckpoint currently returns silently when uploadServiceBinder is null (uploadServiceBinder?.uploadCheckpointDao ?: return), which can let uploads proceed without checkpoint protection; change that early-return to log a warning/error using the same logger used by startFileUpload/startFolderUpload (e.g., log.warn or log.info as those methods do) including vaultId, type and targetFolderPath for context, then return; update the null-check around uploadServiceBinder/uploadCheckpointDao in createUploadCheckpoint to emit that log before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java`:
- Around line 134-150: createCheckpointCallback currently initializes
uploadedSoFar as an empty set which drops previously-completed files across
resumes; change createCheckpointCallback to accept the existing completedFiles
(e.g., Set<String> completedFiles) and initialize uploadedSoFar from a defensive
copy of that set, then update all call sites that invoke
createCheckpointCallback(vaultId) to pass the session's completedFiles; keep the
rest of the callback logic (toJsonArray,
uploadCheckpointDao.updateCompletedFiles, Handler/notification) unchanged.
---
Duplicate comments:
In `@presentation/src/main/java/org/cryptomator/presentation/CryptomatorApp.kt`:
- Around line 227-229: toJsonArray currently does manual string building and
only escapes double quotes, missing backslashes and other JSON-escaping rules;
replace its implementation in the CryptomatorApp.toJsonArray function to use
org.json.JSONArray for proper serialization (construct a new JSONArray(items)
and return its toString()), so the produced JSON matches the parser and
correctly escapes special characters.
In
`@presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java`:
- Around line 152-156: The toJsonArray method currently hand-escapes only double
quotes and is fragile; replace its manual serialization with proper JSON
construction using org.json.JSONArray: create a new JSONArray, add each element
from the Set<String> (preserving strings as-is so JSONArray handles escaping),
and return JSONArray.toString() from toJsonArray to ensure correct escaping of
backslashes and all JSON-special characters (update any imports and references
to toJsonArray accordingly).
- Line 47: The elapsedTimeNotificationDelay field in UploadService is
initialized to 0L which prevents the very first progress notification from
firing; change its initialization so it looks like a past timestamp older than
the progress update threshold (e.g., set elapsedTimeNotificationDelay to
System.currentTimeMillis() - (PROGRESS_UPDATE_THRESHOLD_MS + 1) or equivalent)
so the first callback treats the last-notified time as sufficiently old and
sends an immediate update; apply the same change to the other occurrence used
around the progress-notification logic (the block mentioned at 158-168) so both
places use the past-timestamp initialization.
---
Nitpick comments:
In `@presentation/src/main/java/org/cryptomator/presentation/CryptomatorApp.kt`:
- Around line 206-221: createUploadCheckpoint currently returns silently when
uploadServiceBinder is null (uploadServiceBinder?.uploadCheckpointDao ?:
return), which can let uploads proceed without checkpoint protection; change
that early-return to log a warning/error using the same logger used by
startFileUpload/startFolderUpload (e.g., log.warn or log.info as those methods
do) including vaultId, type and targetFolderPath for context, then return;
update the null-check around uploadServiceBinder/uploadCheckpointDao in
createUploadCheckpoint to emit that log before returning.
presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/ResumeUploadDialog.kt (1)
7-9: Consider a safer callback binding thancontext as Callback.The unchecked cast on line 9 throws a
ClassCastExceptionat construction with no context-specific message ifcontextdoesn't implementCallback. The error is no more descriptive than "VaultListActivity cannot be cast to ResumeUploadDialog$Callback", making future misuse hard to diagnose.Suggested improvement
- private val callback: Callback = context as Callback + private val callback: Callback = requireNotNull(context as? Callback) { + "${context.javaClass.simpleName} must implement ResumeUploadDialog.Callback" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/org/cryptomator/presentation/ui/dialog/ResumeUploadDialog.kt` around lines 7 - 9, The constructor for ResumeUploadDialog is performing an unsafe cast using "context as Callback" which can throw an opaque ClassCastException; update the binding in the ResumeUploadDialog constructor to perform a safe check and throw a clear, descriptive exception (or accept a Callback parameter) instead of relying on the unchecked cast—e.g., use a safe cast (context as? Callback) and if null throw IllegalArgumentException with a message naming ResumeUploadDialog and Callback, or change the constructor signature to accept a Callback directly; update any callers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt`:
- Around line 1080-1083: The code currently calls
context().contentResolver.takePersistableUriPermission(treeUri,
Intent.FLAG_GRANT_READ_URI_PERMISSION) and DocumentFile.fromTreeUri(context(),
treeUri) before the background thread's try/catch, which can throw and crash the
presenter; wrap the takePersistableUriPermission call in a
try/catch(SecurityException) and handle the exception (e.g., log and continue
without persisting the grant), and wrap DocumentFile.fromTreeUri in a
try/catch(Exception) to handle any creation errors (e.g., log, show user
feedback, and return early) so that lastSelectedFolderUri and downstream logic
only proceed when these operations succeed.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt`:
- Around line 569-593: In onResumeUploadConfirmed, the early returns when dao is
null or checkpoint is null leave pendingNavigationAfterResume set and never
consumed; update the two early-return paths (the checks of
getUploadCheckpointDao() and dao.findByVaultId(vaultId)) to call
navigatePendingAfterResume() before returning so pendingNavigationAfterResume is
cleared/consumed (keep the existing Timber/error handling behavior for the vault
load failure path which already calls navigatePendingAfterResume()); this
targets the onResumeUploadConfirmed function and variables cryptomatorApp, dao,
checkpoint, and pendingNavigationAfterResume.
- Around line 604-616: The current try block in handleFolderResume only catches
SecurityException so other exceptions from buildUploadFolderStructure propagate;
change the catch to catch Exception (or add an additional catch (e: Exception))
around DocumentFile.fromTreeUri/buildUploadFolderStructure and handle it the
same way as the SecurityException (log via Timber.tag("VaultListPresenter").w(e,
...), call view?.showMessage(R.string.dialog_resume_upload_permission_lost), and
dao.deleteByVaultId(vaultId)); ensure after the exception handling you still
invoke navigatePendingAfterResume() (same flow as when
BrowseFilesPresenter.selectedFolder handles errors) so the resume flow continues
correctly.
---
Duplicate comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt`:
- Around line 658-666: In parseJsonArray, don't swallow the caught JSONException
silently; update the catch block to log the exception (including the exception
object and a clear message like "Failed to parse JSON array in parseJsonArray")
using the class's logger (or create one if absent) and then return emptyList()
as the safe fallback; keep the method signature and behavior otherwise unchanged
so malformed checkpoint data is recorded for diagnostics while still returning
an empty list.
In
`@presentation/src/main/java/org/cryptomator/presentation/service/UploadService.java`:
- Line 47: The field elapsedTimeNotificationDelay initialized to 0 causes
updateNotification to treat the first call as if a previous timestamp exists and
skip posting; change the logic so the first update always posts by initializing
elapsedTimeNotificationDelay to a sentinel (e.g., -1) or update
updateNotification to check for a "no previous value" case (compare against
sentinel) and always post on the first call, then set/update
elapsedTimeNotificationDelay after posting; apply the same fix to the other
similar block referenced (the logic in the second updateNotification-like
section).
---
Nitpick comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/ui/dialog/ResumeUploadDialog.kt`:
- Around line 7-9: The constructor for ResumeUploadDialog is performing an
unsafe cast using "context as Callback" which can throw an opaque
ClassCastException; update the binding in the ResumeUploadDialog constructor to
perform a safe check and throw a clear, descriptive exception (or accept a
Callback parameter) instead of relying on the unchecked cast—e.g., use a safe
cast (context as? Callback) and if null throw IllegalArgumentException with a
message naming ResumeUploadDialog and Callback, or change the constructor
signature to accept a Callback directly; update any callers accordingly.
presentation/src/main/java/org/cryptomator/presentation/presenter/BrowseFilesPresenter.kt
Outdated
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt
Show resolved
Hide resolved
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.
Adds the ability to upload an entire local folder (with its subfolder structure preserved) into a vault. Users select a folder via the Android document picker, the directory tree is traversed in the background, and vault folders are recursively created before uploading all files. Also extracts duplicated stream-copy utilities into a shared StreamHelper.
Both file and folder uploads now run in a foreground service so they survive app backgrounding and vault locks. Upload progress is checkpointed per file in a new database table, and after vault unlock the app checks for interrupted uploads and offers to resume them.
Closes #63