Add "Make a copy and Select" option into built-in File Picker #293
Conversation
WalkthroughThe pull request introduces a comprehensive enhancement to the file handling and user interface components of the application. Key changes include the implementation of a new option dialog system, improvements to file system operations, and refinements to view models and cell models. The modifications focus on providing more robust file management capabilities, introducing long-click support for file items, and creating a more flexible dialog mechanism for presenting user options. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (16)
app/src/main/res/layout/cell_option_one_line.xml (2)
6-11: Consider adding documentation for the ViewModel binding.While the binding is correctly implemented, adding a comment describing the purpose and usage of
OneLineTextCellViewModelwould improve maintainability.<data> + <!-- OneLineTextCellViewModel handles the presentation logic for single-line option items + in the option dialog, managing text display and click events. --> <variable name="viewModel" type="com.ivanovsky.passnotes.presentation.core.viewmodel.OneLineTextCellViewModel" /> </data>
13-26: Enhance accessibility and RTL support.The TextView implementation is good, but consider these improvements for better accessibility and internationalization:
<TextView style="@style/Material.ListItem.PrimaryText.SingleLine" android:layout_width="match_parent" android:layout_height="@dimen/material_list_item_height_single_line_with_icon" android:background="?android:attr/selectableItemBackground" android:clickable="true" android:focusable="true" android:gravity="center_vertical" android:onClick="@{() -> viewModel.onClicked()}" - android:paddingLeft="@dimen/material_list_item_horizontal_padding" - android:paddingRight="@dimen/material_list_item_horizontal_padding" + android:paddingStart="@dimen/material_list_item_horizontal_padding" + android:paddingEnd="@dimen/material_list_item_horizontal_padding" android:text="@{viewModel.model.text}" android:textStyle="bold" + android:contentDescription="@{viewModel.model.text}" tools:text="Primary text" />app/src/main/kotlin/com/ivanovsky/passnotes/presentation/unlock/UnlockViewModel.kt (1)
384-410: Improve error handling and add clarity to edge cases.While the refactoring improves the code structure, consider these enhancements:
- Add a comment explaining the early return condition
- Provide more context in the IllegalStateException
- Consider handling errors more gracefully instead of throwing exceptions
val keyFile = selectedKeyFile -if (selectedUsedFile.keyType == KeyType.KEY_FILE && keyFile == null) return +// Early return if key file is required but not provided +if (selectedUsedFile.keyType == KeyType.KEY_FILE && keyFile == null) { + setErrorPanelState(OperationError.newError("Key file is required but not provided")) + return +} setScreenState(ScreenState.loading()) viewModelScope.launch { val decodePasswordResult = interactor.decodePassword(decoder, biometricData) if (decodePasswordResult.isFailed) { setErrorPanelState(decodePasswordResult.error) return@launch } val password = decodePasswordResult.getOrThrow() val keyType = selectedUsedFile.keyType val key = when { keyType == KeyType.PASSWORD -> PasswordKeepassKey(password) keyType == KeyType.KEY_FILE && keyFile != null -> FileKeepassKey( file = keyFile, password = password ) - else -> throw IllegalStateException() + else -> throw IllegalStateException("Invalid key type configuration: keyType=$keyType, keyFile=${keyFile != null}") }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/viewmodel/TwoLineTextCellViewModel.kt (1)
17-20: Consider using string template for event constant.The event constant could be more readable using string template syntax.
- val CLICK_EVENT = TwoLineTextCellViewModel::class.qualifiedName + "_clickEvent" + val CLICK_EVENT = "${TwoLineTextCellViewModel::class.qualifiedName}_clickEvent"app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/factory/OptionDialogCellModelFactory.kt (1)
24-35: Consider using a more robust ID generation strategy.Using the index as a string ID might lead to conflicts if the list is modified or if IDs are used across different lists. Consider using a more unique identifier generation strategy.
Apply this diff to improve ID generation:
- id = index.toString(), + id = "${item.hashCode()}_$index",This change makes the IDs more unique by combining the item's hash code with the index.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialogViewModel.kt (1)
63-73: Consider dependency injection alternatives to GlobalInjector.Using
GlobalInjectordirectly in the Factory class could make testing more difficult and creates a tight coupling. Consider using constructor injection or a provider pattern instead.Apply this diff to improve dependency injection:
class Factory( - private val args: OptionDialogArgs + private val args: OptionDialogArgs, + private val modelFactory: OptionDialogCellModelFactory, + private val viewModelFactory: OptionDialogCellViewModelFactory ) : ViewModelProvider.Factory { @Suppress("UNCHECKED_CAST") override fun <T : ViewModel> create(modelClass: Class<T>): T { - return GlobalInjector.get<OptionDialogViewModel>( - parametersOf(args) - ) as T + return OptionDialogViewModel( + modelFactory, + viewModelFactory, + args + ) as T } }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialog.kt (2)
37-37: Consider using a weak reference or clearing the callback.The
onItemClickcallback is stored directly in the fragment, which could potentially cause memory leaks if the dialog is dismissed but the callback reference is retained.Consider using a weak reference or clearing the callback in
onDestroy:- private var onItemClick: ((index: Int) -> Unit)? = null + private var onItemClick = WeakReference<((index: Int) -> Unit)?>(null) + override fun onDestroy() { + super.onDestroy() + onItemClick.clear() + }
89-106: Add KDoc documentation for the factory method.Consider adding documentation to clarify the purpose and usage of the factory method.
Add documentation like this:
+ /** + * Creates a new instance of OptionDialog. + * + * @param args The arguments to configure the dialog + * @param onItemClick Callback invoked when an item is selected + * @return A new instance of OptionDialog + */ fun newInstance( args: OptionDialogArgs, onItemClick: (index: Int) -> Unit ): OptionDialog =app/src/main/kotlin/com/ivanovsky/passnotes/injection/modules/UiModule.kt (1)
213-214: Consider grouping related dialog components.The dialog-related factory definitions are separated. Consider grouping them together for better organization.
Move the option dialog factories near other dialog-related components:
- single { OptionDialogCellModelFactory(get()) } - single { OptionDialogCellViewModelFactory(get()) } // ViewModels + // Dialog ViewModels + single { OptionDialogCellModelFactory(get()) } + single { OptionDialogCellViewModelFactory(get()) } factory { (args: OptionDialogArgs) -> OptionDialogViewModel( get(), get(), args ) }Also applies to: 392-398
app/src/main/res/layout/core_base_cell_dialog.xml (1)
28-37: Add content description for accessibility.The RecyclerView should have a content description for better accessibility support.
Add content description:
<androidx.recyclerview.widget.RecyclerView android:id="@+id/recyclerView" android:layout_width="0dp" android:layout_height="wrap_content" + android:contentDescription="@string/options_list" android:orientation="vertical" app:layoutManager="androidx.recyclerview.widget.LinearLayoutManager"app/src/main/res/layout/cell_option_two_line.xml (1)
21-47: Enhance accessibility and verify RTL support.The TextViews should have content descriptions for accessibility, and the layout should be verified for RTL support.
Add accessibility support and verify RTL:
<TextView android:id="@+id/title" style="@style/Material.PrimaryText.SingleLine" android:layout_width="match_parent" android:layout_height="wrap_content" - android:layout_marginHorizontal="@dimen/element_margin" + android:layout_marginStart="@dimen/element_margin" + android:layout_marginEnd="@dimen/element_margin" + android:importantForAccessibility="yes" android:text="@{viewModel.model.title}" android:textStyle="bold" tools:text="Title" /> <TextView android:id="@+id/description" style="@style/Material.SecondaryText" android:layout_width="0dp" android:layout_height="wrap_content" + android:importantForAccessibility="yes" android:text="@{viewModel.model.description}" tools:text="Description" />app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemProvider.kt (1)
170-187: Verify parent directory existence before file creation.While the file existence check is good, consider verifying that the parent directory exists and is writable before attempting to create the file.
return lock.withLock { val isExistResult = exists(file) if (isExistResult.isFailed) { return@withLock isExistResult.takeError() } val isExist = isExistResult.getOrThrow() if (isExist && onConflictStrategy == OnConflictStrategy.CANCEL) { return@withLock OperationResult.error(newFileIsAlreadyExistsError()) } + val parent = File(file.path).parentFile + if (parent != null && !parent.exists() && !parent.mkdirs()) { + return@withLock OperationResult.error( + newGenericIOError("Failed to create parent directory") + ) + } + try { val out = BufferedOutputStream(FileOutputStream(file.path)) OperationResult.success(out) } catch (e: Exception) { Timber.d(e) OperationResult.error(newGenericIOError(e.message, e)) } }app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/filepicker/FilePickerInteractor.kt (1)
81-90: Consider adding timeout for file operations.While the implementation is correct, long-running file operations could potentially block the UI thread.
Consider adding a timeout parameter with a default value:
-private suspend fun openFile(file: FileDescriptor): OperationResult<InputStream> = +private suspend fun openFile( + file: FileDescriptor, + timeoutMs: Long = 30000 +): OperationResult<InputStream> = withContext(dispatchers.IO) { - val fsProvider = fileSystemResolver.resolveProvider(file.fsAuthority) + withTimeout(timeoutMs) { + val fsProvider = fileSystemResolver.resolveProvider(file.fsAuthority) - fsProvider.openFileForRead( - file, - OnConflictStrategy.CANCEL, - FSOptions.READ_ONLY - ) + fsProvider.openFileForRead( + file, + OnConflictStrategy.CANCEL, + FSOptions.READ_ONLY + ) + } }app/src/main/kotlin/com/ivanovsky/passnotes/util/InputOutputUtils.kt (1)
133-144: Consider adding progress tracking for large files.For large file operations, it would be helpful to track and report progress.
Consider adding a progress callback:
fun copyOrThrow( from: InputStream, to: OutputStream, isCloseOnFinish: Boolean, - cancellation: AtomicBoolean + cancellation: AtomicBoolean, + onProgress: ((bytesTransferred: Long) -> Unit)? = null ) { try { val buf = ByteArray(BUFFER_SIZE) var len: Int + var totalBytesTransferred: Long = 0 while (from.read(buf).also { len = it } > 0 && !cancellation.get()) { to.write(buf, 0, len) + totalBytesTransferred += len + onProgress?.invoke(totalBytesTransferred) } to.flush() } finally {app/src/main/kotlin/com/ivanovsky/passnotes/presentation/filepicker/FilePickerFragment.kt (1)
169-191: Consider handling dialog dismissal.The dialog implementation should handle dismissal gracefully.
Add dialog dismissal handling:
private fun showFileMenuDialog(items: List<FileMenuItem>) { val options = items.map { item -> when (item) { FileMenuItem.SELECT -> OptionItem( title = resources.getString(R.string.select), description = null ) FileMenuItem.COPY_AND_SELECT -> OptionItem( title = resources.getString(R.string.make_a_copy_and_select), description = resources.getString(R.string.copy_and_select_description) ) } } val dialog = OptionDialog.newInstance( args = OptionDialogArgs(options), onItemClick = { index -> viewModel.onFileMenuClicked(items[index]) - } + }, + onDismiss = { + viewModel.onFileMenuDismissed() + } ) dialog.show(childFragmentManager, OptionDialog.TAG) }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/filepicker/FilePickerViewModel.kt (1)
153-167: Consider adding progress indication for file copy operations.Long-running copy operations should show progress to improve user experience.
Add progress tracking:
private fun copyAndSelectFile(file: FileDescriptor) { setScreenState(ScreenState.loading()) viewModelScope.launch { - val copyResult = interactor.copyToPrivateStorage(file) + val copyResult = interactor.copyToPrivateStorage( + file = file, + onProgress = { bytesTransferred -> + setScreenState( + ScreenState.loading( + progress = bytesTransferred, + total = file.size + ) + ) + } + ) if (copyResult.isFailed) { val message = errorInteractor.processAndGetMessage(copyResult.error) setScreenState(ScreenState.dataWithError(message)) return@launch } val copiedFile = copyResult.getOrThrow() selectFile(copiedFile) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemProvider.kt(5 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/filepicker/FilePickerInteractor.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/injection/modules/UiModule.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialog.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialogArgs.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialogViewModel.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/factory/OptionDialogCellModelFactory.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/factory/OptionDialogCellViewModelFactory.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/model/OptionItem.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/model/OneLineTextCellModel.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/model/TwoLineTextCellModel.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/viewmodel/FileCellViewModel.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/viewmodel/OneLineTextCellViewModel.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/viewmodel/TwoLineTextCellViewModel.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/filepicker/FilePickerFragment.kt(4 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/filepicker/FilePickerViewModel.kt(8 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/GroupsViewModel.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/dialog/ChooseOptionDialog.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/storagelist/StorageListViewModel.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/storagelist/factory/StorageListCellModelFactory.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/storagelist/factory/StorageListCellViewModelFactory.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/unlock/UnlockViewModel.kt(4 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/util/InputOutputUtils.kt(4 hunks)app/src/main/res/layout/cell_file.xml(1 hunks)app/src/main/res/layout/cell_option_one_line.xml(1 hunks)app/src/main/res/layout/cell_option_two_line.xml(1 hunks)app/src/main/res/layout/cell_single_text.xml(1 hunks)app/src/main/res/layout/core_base_cell_dialog.xml(1 hunks)app/src/main/res/values/strings.xml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/model/OneLineTextCellModel.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/storagelist/StorageListViewModel.kt
🧰 Additional context used
🪛 detekt (1.23.7)
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/GroupsViewModel.kt
[warning] 605-605: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
🔇 Additional comments (30)
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/viewmodel/OneLineTextCellViewModel.kt (2)
6-6: LGTM! Class and model type renamed consistently.The renaming from
SingleTextCellViewModeltoOneLineTextCellViewModeland corresponding model type update is consistent with the refactoring effort.Also applies to: 8-9
19-19: LGTM! Event constant updated correctly.The event constant has been updated to use the new class name while maintaining the same pattern.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/storagelist/factory/StorageListCellViewModelFactory.kt (1)
7-7: LGTM! Factory updated consistently with renamed types.The import statements and model type check in the factory have been updated correctly to use the new class names.
Also applies to: 9-9, 19-19
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/storagelist/factory/StorageListCellModelFactory.kt (1)
8-8: LGTM! Model factory updated consistently with renamed types.The import statement and model instantiation have been updated correctly to use the new class name.
Also applies to: 34-34
app/src/main/res/layout/cell_single_text.xml (1)
10-10: LGTM! Layout data binding updated consistently.The ViewModel type in the data binding has been updated correctly to use the new class name.
app/src/main/res/layout/cell_option_one_line.xml (1)
1-4: LGTM!The XML declaration and layout structure follow Android data binding best practices.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/unlock/UnlockViewModel.kt (3)
36-36: LGTM!The addition of
getOrThrowextension function import improves error handling in the biometric unlock flow.
200-201: LGTM!The simplified biometric authentication check improves code readability while maintaining the same functionality.
899-900: LGTM!The multi-line formatting of the condition improves code readability.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/GroupsViewModel.kt (2)
25-25: LGTM!The import is correctly added and necessary for handling the new key type.
1322-1322: Verify biometric unlock support for all key types.The condition has been simplified to support biometric unlock for all key types, which aligns with the support for both password and file-based keys.
Run the following script to verify biometric unlock support:
✅ Verification successful
Biometric unlock properly supports all key types
The implementation correctly handles biometric unlock for both password-based and key file-based authentication methods. The biometric authentication process is key-type agnostic and properly integrates with both authentication flows after successful biometric verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that biometric unlock is properly handled for all key types. # Test: Search for key type checks in biometric-related code. rg -A 5 'biometric|KeyType' --type kotlinLength of output: 81079
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/viewmodel/FileCellViewModel.kt (2)
17-19: LGTM! Clean implementation of long click handling.The
onLongClicked()method follows the same pattern as the existingonClicked()method, maintaining consistency in event handling.
21-24: LGTM! Consistent constant naming.The
LONG_CLICK_EVENTconstant follows the same naming pattern asCLICK_EVENT, maintaining code consistency.app/src/main/res/layout/cell_file.xml (1)
21-22: Verify the long click binding implementation.The long click binding looks correct, but please ensure:
- The binding adapter for
app:onLongClickis properly registered- The long click handler doesn't interfere with the regular click handler
✅ Verification successful
Long click binding implementation is correct
The binding adapter for
app:onLongClickis properly registered and implemented inBindingAdapters.kt. The implementation correctly handles event consumption and won't interfere with the regular click handler.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for binding adapter registration for onLongClick # Search for binding adapter ast-grep --pattern 'class $_ { $$$ @BindingAdapter("onLongClick") fun $_(view: View, $handler: $type) { $$$ } $$$ }' # Search for potential conflicts in click handling rg -A 5 'onLongClick.*onClicked'Length of output: 167
Script:
#!/bin/bash # Broader search for long click related code # Search for binding adapters with different naming patterns ast-grep --pattern '@BindingAdapter($pattern) fun $_($$$) { $$$ }' # Search for any long click related code rg -i "longclick|long_click" -A 5 # Search for binding utility files fd -e kt -e java "bind|binding" --exec cat {}Length of output: 41520
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/model/TwoLineTextCellModel.kt (1)
3-7: LGTM! Well-structured data class implementation.The data class follows Kotlin best practices with clear property definitions and appropriate inheritance from
BaseCellModel.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/model/OptionItem.kt (1)
6-10: LGTM! Well-structured Parcelable data class implementation.The data class follows Kotlin best practices with appropriate use of
@Parcelizeannotation and clear property definitions.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialogArgs.kt (1)
7-10: LGTM! Well-structured Parcelable data class implementation.The data class follows Kotlin best practices with appropriate use of
@Parcelizeannotation and clear property definition for the options list.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/viewmodel/TwoLineTextCellViewModel.kt (1)
8-15: LGTM! Well-structured ViewModel implementation.The class follows MVVM pattern best practices with clear separation of concerns and proper event handling.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/dialog/ChooseOptionDialog.kt (1)
10-10: LGTM! Appropriate deprecation notice.The deprecation notice clearly indicates the replacement class to use.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/factory/OptionDialogCellViewModelFactory.kt (1)
15-28: LGTM! Well-structured factory implementation.The factory correctly handles all cell model types with appropriate error handling for unsupported models.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialogViewModel.kt (1)
34-46: LGTM! Clean event handling implementation.The event subscription is well-structured with proper null safety checks and clear event handling logic.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialog.kt (1)
47-74: LGTM! Well-structured view setup.The view setup follows Android best practices with proper theming and adapter configuration.
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemProvider.kt (2)
15-15: LGTM!The new import is correctly placed and necessary for the enhanced error handling.
111-115: Consider validating external storage root path.While using
firstOrNull()is safe, consider adding validation to ensure the selected external storage path is accessible and writable.Run this script to check external storage paths:
app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/filepicker/FilePickerInteractor.kt (1)
35-79: LGTM! Well-structured implementation of copyToPrivateStorage.The implementation follows best practices with:
- Proper error handling and mapping
- Clear separation of concerns
- Resource cleanup
app/src/main/kotlin/com/ivanovsky/passnotes/util/InputOutputUtils.kt (1)
42-59: LGTM! Well-implemented stream copy operation.The implementation includes:
- Proper resource management
- Comprehensive error handling
- Thread-safe operation
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/filepicker/FilePickerFragment.kt (1)
151-153: LGTM! Clean event observation implementation.The event handling follows MVVM pattern correctly.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/filepicker/FilePickerViewModel.kt (2)
132-139: LGTM! Clean implementation of file menu handling.The menu handling follows MVVM pattern with proper null checks.
350-370: 🛠️ Refactor suggestionConsider adding file size check before copy operation.
To prevent potential issues with large files, add a size check before copying.
Add size validation:
private fun onItemLongClicked(filePath: String?) { val file = filePathToFileMap[filePath] ?: return selectedFile = file val menuItems = mutableListOf<FileMenuItem>() if ((args.action == Action.PICK_FILE && !file.isDirectory) || (args.action == Action.PICK_DIRECTORY && file.isDirectory) ) { menuItems.add(FileMenuItem.SELECT) } - if (!file.isDirectory && file.fsAuthority.type != FSType.INTERNAL_STORAGE) { + if (!file.isDirectory && + file.fsAuthority.type != FSType.INTERNAL_STORAGE && + file.size <= MAX_COPY_FILE_SIZE + ) { menuItems.add(FileMenuItem.COPY_AND_SELECT) } if (menuItems.isNotEmpty()) { showFileMenuDialog.call(menuItems) } } +private companion object { + const val MAX_COPY_FILE_SIZE = 100 * 1024 * 1024L // 100MB +}✅ Verification successful
Add size check to prevent internal storage issues
The suggestion to add a size check is valid. The code copies files to internal storage without any size limits, which could lead to storage space issues. The proposed 100MB limit is reasonable for internal storage operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any size limits defined in the codebase # Search for size-related constants rg -i 'size|limit|max.*bytes' --type kotlinLength of output: 17750
Script:
#!/bin/bash # Find the implementation of copy operation ast-grep --pattern 'fun $COPY_METHOD($$$) { $$$ }' # Also search for any copy-related methods rg -g '*.kt' -A 5 'copy.*select|COPY_AND_SELECT'Length of output: 68760
Script:
#!/bin/bash # Find COPY_AND_SELECT definition and usage rg -g '*.kt' -B 2 -A 2 'enum.*FileMenuItem|COPY_AND_SELECT' # Find where FileMenuItem is handled ast-grep --pattern 'when ($expr) { $$$ FileMenuItem.COPY_AND_SELECT -> $$$ $$$ }'Length of output: 2805
Script:
#!/bin/bash # Find copyAndSelectFile implementation ast-grep --pattern 'fun copyAndSelectFile($$$) { $$$ }' # Also search for any copy utilities rg -g '*.kt' -B 2 -A 10 'fun.*copy.*file|class.*copy.*file'Length of output: 3510
Script:
#!/bin/bash # Get the complete implementation of copyToPrivateStorage rg -g '*.kt' -B 5 -A 30 'fun copyToPrivateStorage' # Find any file copy utilities rg -g '*.kt' 'fun.*copy.*file|class.*copy.*file|BUFFER_SIZE|copy.*stream'Length of output: 5472
app/src/main/res/values/strings.xml (1)
293-294: LGTM! Well-structured string resources.The new string resources are properly organized, follow naming conventions, and provide clear user-facing text that aligns with the PR's objective of adding a "Make a copy and Select" option to the built-in File Picker.
| val password = when (val key = getKeyResult.getOrThrow()) { | ||
| is PasswordKeepassKey -> key.password | ||
| is FileKeepassKey -> key.password | ||
| else -> throw IllegalStateException() | ||
| } ?: EMPTY |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a descriptive message to IllegalStateException.
The type handling is good, but the exception should include a message explaining why it was thrown.
Apply this diff to improve error handling:
- else -> throw IllegalStateException()
+ else -> throw IllegalStateException("Unsupported key type: ${key::class.simpleName}")📝 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.
| val password = when (val key = getKeyResult.getOrThrow()) { | |
| is PasswordKeepassKey -> key.password | |
| is FileKeepassKey -> key.password | |
| else -> throw IllegalStateException() | |
| } ?: EMPTY | |
| val password = when (val key = getKeyResult.getOrThrow()) { | |
| is PasswordKeepassKey -> key.password | |
| is FileKeepassKey -> key.password | |
| else -> throw IllegalStateException("Unsupported key type: ${key::class.simpleName}") | |
| } ?: EMPTY |
🧰 Tools
🪛 detekt (1.23.7)
[warning] 605-605: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
...rc/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialog.kt
Show resolved
Hide resolved
...ain/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemProvider.kt
Show resolved
Hide resolved
aivanovski
deleted the
feature/add-option-to-copy-db-key-into-private-storage
branch
Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
New Features
Improvements
Bug Fixes
Refactoring
UI Changes
These release notes provide a high-level overview of the significant changes while maintaining confidentiality about internal implementation details.