Refactor error displaying and catching#296
Conversation
WalkthroughThe changes implement a comprehensive refactor across the project. The app version is incremented and a new Compose icons dependency is added. Error handling has been overhauled: many repository methods and UI view models now include stack trace information by accepting a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant VM as ViewModel
participant OE as OperationError
participant RP as ResourceProvider
participant UI as ErrorPanelView
U->>VM: Trigger action that causes an error
VM->>OE: Create error via newGenericError(message, Stacktrace())
OE->>RP: Format error message via formatReadableMessage(...)
RP-->>VM: Return formatted error message
VM->>UI: Update screen state with error message
Poem
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (81)
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/TextField.kt (1)
152-161: LGTM! Consider aligning default font size with secondary metrics.The addition of the
fontSizeparameter with a default value enhances the flexibility of theSecondaryTextStylecomposable while maintaining backward compatibility. However, since this is a "secondary" text style, consider usingAppTheme.theme.textMetrics.secondaryas the default value to align with the pattern seen inSecondaryMonospaceTextStyle.- fontSize: TextUnit = AppTheme.theme.textMetrics.primary, + fontSize: TextUnit = AppTheme.theme.textMetrics.secondary,app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/extensions/BundleExt.kt (1)
8-18: Consider adding KDoc documentation.Add documentation to explain:
- Purpose and usage examples
- Reason for suppressions
- API level compatibility
+/** + * Type-safe extension function to retrieve Serializable objects from Bundle. + * Handles API level differences by using the new type-safe method on Android 13+ + * and falling back to traditional casting for older versions. + * + * @param key The key to retrieve the serializable object + * @param type The KClass representing the expected type + * @return The serializable object of type T, or null if not found + * + * @suppress Deprecation warning is suppressed for backward compatibility with + * older Android versions. Unchecked cast is unavoidable for API < 33. + * + * Example usage: + * ``` + * val error = bundle.getSerializableCompat("error", OperationError::class) + * ``` + */ @Suppress("DEPRECATION", "UNCHECKED_CAST") fun <T : Serializable> Bundle.getSerializableCompat(app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/FindParentGroupsUseCase.kt (1)
38-41: Consider reusing Stacktrace instance.The error handling follows the new pattern correctly. However, since there are multiple error cases in this method, consider creating a single Stacktrace instance at the beginning of the method to avoid creating multiple instances.
suspend fun findAllParents( groupUid: UUID, isAddCurrentGroup: Boolean ): OperationResult<List<Group>> { return withContext(dispatchers.IO) { + val stacktrace = Stacktrace() val getDbResult = getDbUseCase.getDatabaseSynchronously() // ... rest of the code ... ?: return@withContext OperationResult.error( newDbError( MESSAGE_FAILED_TO_FIND_ROOT_GROUP, - Stacktrace() + stacktrace ) )app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/GroupsFragment.kt (1)
285-290: Address TODO comments: Move menu creation to ViewModel.There are duplicate TODO comments about moving menu creation to the ViewModel. This refactoring would improve separation of concerns by:
- Moving menu item creation logic to the ViewModel
- Eliminating duplicate code in
showGroupActionsDialogandshowNoteActionsDialog- Making the UI layer thinner and more focused on display logic
Would you like me to help implement this refactoring by:
- Creating a new issue to track this improvement
- Generating the refactored code that moves menu creation to the ViewModel?
Also applies to: 299-304
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/diffViewer/DiffViewerInteractor.kt (1)
51-56: LGTM with a suggestion for improvement.The error handling enhancement with stack trace information is well implemented. However, consider extracting the error creation to a private method since the same error is created in both
getOpenedDatabaseAndFileandreadDatabasemethods.+ private fun createUnsupportedDatabaseError() = + newGenericError( + MESSAGE_UNSUPPORTED_DATABASE_TYPE, + Stacktrace() + ) + suspend fun getOpenedDatabaseAndFile(): OperationResult<Pair<KotpassDatabase, FileDescriptor>> = withContext(dispatchers.IO) { // ... } else { - OperationResult.error( - newGenericError( - MESSAGE_UNSUPPORTED_DATABASE_TYPE, - Stacktrace() - ) - ) + OperationResult.error(createUnsupportedDatabaseError()) } }app/src/main/kotlin/com/ivanovsky/passnotes/domain/test/TestDataBroadcastReceiver.kt (1)
24-30: Consider enhancing error logging with stack traces.While the error handling is correct, consider including stack traces in the error logging for better debugging capabilities. This would align with the broader refactoring effort where stack trace information has been added to error handling across the codebase.
if (parseCommandResult.isFailed) { val message = parseCommandResult.error.formatReadableMessage(resourceProvider) - Timber.e(message) + Timber.e(parseCommandResult.error.cause ?: Exception(message), message) showToast(context, message) return }Similar enhancement for the second error block:
if (processCommandResult.isFailed) { val message = processCommandResult.error.formatReadableMessage(resourceProvider) - Timber.e(message) + Timber.e(processCommandResult.error.cause ?: Exception(message), message) showToast(context, message) return }Also applies to: 32-39
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemSyncProcessor.kt (1)
44-51: Consider reusing the Stacktrace instance.Since both methods return the same error with a new stack trace, consider creating a single instance at the class level to avoid redundant stack trace captures.
class RegularFileSystemSyncProcessor( private val provider: RegularFileSystemProvider ) : FileSystemSyncProcessor { + private val incorrectUseCaseError = newGenericError( + MESSAGE_INCORRECT_USE_CASE, + Stacktrace() + ) override fun getSyncConflictForFile(uid: String): OperationResult<SyncConflictInfo> { - return OperationResult.error( - newGenericError( - MESSAGE_INCORRECT_USE_CASE, - Stacktrace() - ) - ) + return OperationResult.error(incorrectUseCaseError) }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/app/AppSettingsViewModel.kt (1)
20-128: Consider further improvements to enhance maintainability.While the error handling refactoring is good, there are a few suggestions to improve the code further:
- Consider splitting this ViewModel into smaller, more focused ViewModels (e.g.,
LogSettingsViewModel,BiometricSettingsViewModel,NotificationSettingsViewModel).- Consider adding error handling to other methods that perform operations (e.g.,
onBiometricUnlockEnabledChanged,onPostponedSyncEnabledChanged).- Consider wrapping the loading state management in a common pattern across all operations.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/filepicker/FilePickerViewModel.kt (1)
163-180: Consider adding error handling for null directory case.The
onDoneButtonClickedmethod could benefit from error handling when the current directory is null in thePICK_DIRECTORYaction.Consider this improvement:
when (args.action) { Action.PICK_DIRECTORY -> { val currentDir = currentDir ?: return + if (!currentDir.isDirectory) { + showSnackbarMessageEvent.call( + resourceProvider.getString(R.string.please_select_valid_directory) + ) + return + } selectFileAndExit(currentDir) }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/DefaultScreenVisibilityHandler.kt (3)
17-43: Add KDoc documentation to explain state behaviors.The method would benefit from documentation explaining the expected visibility behavior for each screen state.
Add documentation like this:
+ /** + * Applies visibility rules based on the screen state. + * + * @param view The view whose visibility should be modified + * @param screenState The current screen state + * + * Visibility rules: + * - DATA: All views visible except ScreenStateView and ErrorPanelView + * - DATA_WITH_ERROR: All views visible except ScreenStateView + * - EMPTY: Only ScreenStateView and FloatingActionButton visible + * - LOADING: Only ScreenStateView visible + * - ERROR: Only ScreenStateView visible + * - NOT_INITIALIZED: All views hidden + */ override fun applyScreenState(view: View, screenState: ScreenState) {
31-37: Consider consolidating identical state handling.The LOADING and ERROR states have identical visibility logic. Consider combining them to improve maintainability:
- LOADING -> { - view.isVisible = (view is ScreenStateView) - } - - ERROR -> { - view.isVisible = (view is ScreenStateView) - } + LOADING, ERROR -> { + view.isVisible = (view is ScreenStateView) + }
17-17: Add null safety check for screenState parameter.Consider adding a null check for the screenState parameter to prevent potential NullPointerException.
- override fun applyScreenState(view: View, screenState: ScreenState) { + override fun applyScreenState(view: View, screenState: ScreenState?) { + if (screenState == null) { + view.isVisible = false + return + }app/src/main/java/com/ivanovsky/passnotes/data/repository/file/remote/RemoteFileSyncProcessor.java (1)
384-385: Consider reducing error handling duplication.The same error (generic IO with MESSAGE_FAILED_TO_ACCESS_TO_FILE) is created in three places. Consider extracting it to a local variable to reduce duplication.
private OperationResult<InputStream> copyFileAndOpen(File file) { OperationResult<InputStream> result = new OperationResult<>(); + OperationError fileAccessError = newGenericIOError(MESSAGE_FAILED_TO_ACCESS_TO_FILE, new Stacktrace()); File buffer = fileHelper.generateDestinationFileOrNull(); if (buffer != null) { try { FileUtils.copyFile(file, buffer); InputStream in = newFileInputStreamOrNull(buffer); if (in != null) { result.setObj(in); } else { - result.setError( - newGenericIOError(MESSAGE_FAILED_TO_ACCESS_TO_FILE, new Stacktrace())); + result.setError(fileAccessError); } } catch (IOException e) { Timber.d(e); - result.setError( - newGenericIOError(MESSAGE_FAILED_TO_ACCESS_TO_FILE, new Stacktrace())); + result.setError(fileAccessError); } } else { - result.setError(newGenericIOError(MESSAGE_FAILED_TO_ACCESS_TO_FILE, new Stacktrace())); + result.setError(fileAccessError); } return result; }Also applies to: 390-391, 394-394
app/src/main/res/layout/group_editor_fragment.xml (1)
19-27: ErrorPanelView Enhancements and ViewModel Property NamingThe addition of the
stateattribute to bindviewModel.screenStateand the newandroid:background="?kpErrorBackgroundColor"attribute improves the highlighting of error states. Also, the view now receives the newapp:screenVisibilityHandlerattribute. Please verify that binding it toviewModel.screenStateHandleris intentional; if the renaming is meant to be pervasive, you might want to consider updating the ViewModel property name accordingly for clarity and consistency.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/storagelist/StorageListViewModel.kt (2)
87-89: Consider standardizing error message handling.The error handling has been improved with stack traces, but there's an opportunity to standardize error messages:
- Line 88: Direct string usage
- Lines 104, 120, 145, 327: Direct error object usage
Consider creating an error message factory to ensure consistent error formatting across all cases.
Example approach:
-val errorMessage = resourceProvider.getString(R.string.authentication_failed) -setErrorPanelState(OperationError.newErrorMessage(errorMessage, Stacktrace())) +setErrorPanelState(ErrorMessageFactory.createAuthenticationError(resourceProvider, Stacktrace()))Also applies to: 104-105, 120-121, 145-146, 327-328
157-164: Add error handling for file selection callback.The file selection callback lacks error handling for cases where the result is not a
FileDescriptor. Consider adding explicit error handling to improve user feedback.router.setResultListener(FilePickerScreen.RESULT_KEY) { file -> - if (file is FileDescriptor) { - isReloadOnStart = false - onFilePickedByPicker(file) + when (file) { + is FileDescriptor -> { + isReloadOnStart = false + onFilePickedByPicker(file) + } + else -> { + isReloadOnStart = false + setErrorPanelState(OperationError.newErrorMessage( + resourceProvider.getString(R.string.invalid_file_selection), + Stacktrace() + )) + } } }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/model/ServerLoginState.kt (1)
7-24: Consider storing sensitive data more securely and reducing complexity
- Sensitive Data: Storing a plain text password in a public data class can pose risks if the state object is logged or persisted elsewhere. Consider using more secure mechanisms or at least ensuring this data class does not leak the password to logs.
- Numerous Booleans: The state holds many Boolean flags (e.g.,
isUsernameEnabled,isPasswordEnabled). If the screen grows more complex, consider grouping related flags or using a sealed hierarchy to reduce the chance of errors due to Boolean proliferation.- Maintainability: This data class holds a lot of parameters. Large constructors can be more prone to confusion and change. Balancing a single, all-encompassing state object with more modular approaches can improve maintainability.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/resolveConflict/ResolveConflictDialogViewModel.kt (2)
16-17: Validate necessity of newly added imports
You introducedBaseScreenViewModelandDefaultScreenVisibilityHandler. Make sure these imports are essential. IfDefaultScreenVisibilityHandleris not utilized within this view model, consider removing it or adding usage to maintain clarity and avoid dead code.
46-46: Ensure meaningful user feedback on error
UsingsetErrorState(getConflictResult.error)is part of the new error handling approach. Verify that it provides an actionable or clear message for the user, rather than a generic error message. This ensures a better user experience.app/src/main/java/com/ivanovsky/passnotes/data/repository/file/remote/RemoteFileSystemProvider.java (6)
192-193: Consider using FileNotFoundException for consistency.Currently, a new
Stacktraceis created, but for consistency with other error handling in this class (e.g.,createOperationErrorFromException), consider using aFileNotFoundException:- return OperationResult.error( - newGenericIOError(MESSAGE_FAILED_TO_FIND_FILE, new Stacktrace())); + return OperationResult.error( + newGenericIOError(MESSAGE_FAILED_TO_FIND_FILE, + new FileNotFoundException(MESSAGE_FAILED_TO_FIND_FILE)));
288-290: Use caught exceptions for stack traces where available.For consistency with
createOperationErrorFromException, consider using the caught exceptions instead of creating newStacktraceinstances:- return OperationResult.error( - newGenericIOError(ERROR_FAILED_TO_FIND_APP_PRIVATE_DIR, new Stacktrace())); + return OperationResult.error( + newGenericIOError(ERROR_FAILED_TO_FIND_APP_PRIVATE_DIR, + new FileNotFoundException(ERROR_FAILED_TO_FIND_APP_PRIVATE_DIR))); - result.setError( - newGenericIOError( - ERROR_FAILED_TO_START_PROCESSING_UNIT, new Stacktrace())); + result.setError( + newGenericIOError( + ERROR_FAILED_TO_START_PROCESSING_UNIT, e)); - result.setError( - newGenericIOError( - ERROR_FAILED_TO_START_PROCESSING_UNIT, new Stacktrace())); + result.setError( + newGenericIOError( + ERROR_FAILED_TO_START_PROCESSING_UNIT, e)); - result.setError(newNetworkIOError(new Stacktrace())); + result.setError(newNetworkIOError(e));Also applies to: 385-388, 431-434, 437-437
522-523: Use caught exceptions for stack traces in openFileForWrite.Similar to previous suggestions, consider using the caught exceptions instead of creating new
Stacktraceinstances:- return OperationResult.error( - newGenericIOError(MESSAGE_WRITE_OPERATION_IS_NOT_SUPPORTED, new Stacktrace())); + return OperationResult.error( + newGenericIOError(MESSAGE_WRITE_OPERATION_IS_NOT_SUPPORTED, + new UnsupportedOperationException(MESSAGE_WRITE_OPERATION_IS_NOT_SUPPORTED))); - return OperationResult.error( - newGenericIOError(ERROR_FAILED_TO_FIND_APP_PRIVATE_DIR, new Stacktrace())); + return OperationResult.error( + newGenericIOError(ERROR_FAILED_TO_FIND_APP_PRIVATE_DIR, + new FileNotFoundException(ERROR_FAILED_TO_FIND_APP_PRIVATE_DIR))); - result.setError( - newGenericIOError( - ERROR_FAILED_TO_START_PROCESSING_UNIT, new Stacktrace())); + result.setError( + newGenericIOError( + ERROR_FAILED_TO_START_PROCESSING_UNIT, e)); - newGenericIOError( - ERROR_FAILED_TO_START_PROCESSING_UNIT, - new Stacktrace())); + newGenericIOError( + ERROR_FAILED_TO_START_PROCESSING_UNIT, + e)); - newGenericIOError( - ERROR_FAILED_TO_START_PROCESSING_UNIT, - new Stacktrace())); + newGenericIOError( + ERROR_FAILED_TO_START_PROCESSING_UNIT, + e)); - newDbVersionConflictError( - MESSAGE_LOCAL_VERSION_CONFLICTS_WITH_REMOTE, new Stacktrace())); + newDbVersionConflictError( + MESSAGE_LOCAL_VERSION_CONFLICTS_WITH_REMOTE, e));Also applies to: 530-531, 593-595, 650-653, 679-681, 686-687
766-767: Use caught FileNotFoundException for stack trace.For consistency, use the caught exception:
- String.format(ERROR_FAILED_TO_FIND_FILE, file.getLocalPath()), - new Stacktrace())); + String.format(ERROR_FAILED_TO_FIND_FILE, file.getLocalPath()), + e));
948-949: Use FileNotFoundException consistently in openFileInputStream.For consistency in error handling:
- return OperationResult.error( - newGenericIOError(MESSAGE_FAILED_TO_FIND_FILE, new Stacktrace())); + return OperationResult.error( + newGenericIOError(MESSAGE_FAILED_TO_FIND_FILE, + new FileNotFoundException(MESSAGE_FAILED_TO_FIND_FILE))); - return OperationResult.error( - newGenericIOError(MESSAGE_FAILED_TO_FIND_FILE, new Stacktrace())); + return OperationResult.error( + newGenericIOError(MESSAGE_FAILED_TO_FIND_FILE, e));Also applies to: 956-957
1-973: Consider a consistent approach to stack trace creation.While the addition of stack traces to error handling is a good improvement, there's an inconsistency in how they are created. Some methods use actual exceptions (good), while others create new
Stacktraceinstances (less helpful). Consider consistently using actual exceptions where possible, as they provide more context and are more helpful for debugging.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/database/changePassword/ChangePasswordDialogViewModel.kt (1)
47-47: Displaying errors usingsetErrorPanelState.
Be sure that the passedchangeResult.errorincludes enough context for a user-friendly message. If needed, enrich it by using theresourceProviderfor localized text.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/enterDbCredentials/EnterDbCredentialsViewModel.kt (1)
139-140: OverriddensetScreenStateensures extended logic
Callingsuper.setScreenState(state)and then updatingisAddKeyButtonVisibleis a clean extension of the base screen state handling. Consider also reflecting any potential changes toscreenVisibilityHandlerhere, if needed, to remain consistent.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/note/NoteViewModel.kt (1)
1-681: Architectural Recommendation: Consider implementing the Command Pattern for menu actions.The class handles multiple menu actions and screen transitions. Consider implementing the Command Pattern to encapsulate these actions, making the code more maintainable and testable.
Example implementation:
sealed class NoteCommand { data class LockDatabase(val appMode: ApplicationLaunchMode) : NoteCommand() data class Search(val args: GroupsScreenArgs) : NoteCommand() // ... other commands } class NoteCommandHandler( private val router: Router, private val interactor: NoteInteractor ) { fun execute(command: NoteCommand) { when (command) { is NoteCommand.LockDatabase -> { interactor.lockDatabase() // ... handle navigation } // ... handle other commands } } }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/widget/ScreenStateView.kt (2)
13-13: Remove or resolve the TODO comment if it's no longer relevant.
Leaving it in can create confusion. If no further action is needed, consider removing this comment.Do you want me to open a new issue to track potential cleanup tasks?
16-16: Consider providing a default state instead of null.
Currently,var state: ScreenState? = nullcan lead to edge cases ifstateis accessed too early. If a default loading or empty state applies, you may wish to set it here to ensure consistent UI.- var state: ScreenState? = null + var state: ScreenState = ScreenState( + isDisplayingLoading = true + )app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/fake/FakeFileSystemProvider.kt (2)
172-174: Consider capturing the actual stack trace.While the change correctly implements the new error handling pattern, consider capturing the actual stack trace at the point of error instead of creating a new empty one. This would provide more meaningful debugging information.
private fun newAuthError(): OperationError { - return OperationError.newAuthError(Stacktrace()) + return OperationError.newAuthError(Stacktrace.capture()) }
176-184: Consider capturing the actual stack trace for consistency.Similar to the previous comment, consider capturing the actual stack trace at the point of error for more meaningful debugging information.
private fun newFileNotFoundError(pathOrUid: String): OperationError { return OperationError.newFileNotFoundError( String.format( GENERIC_MESSAGE_FAILED_TO_FIND_FILE, pathOrUid ), - Stacktrace() + Stacktrace.capture() ) }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/GroupsViewModel.kt (1)
576-576: Consider standardizing error handling methods.Here,
setErrorPanelStateis used instead ofsetErrorState. Ensure the differences between these two methods are intentional. If both serve the same purpose, merging them into a single method could simplify the code.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/newdb/NewDatabaseViewModel.kt (2)
70-70: Consider passing an actual exception to enrichStacktrace().
Right now it’s a placeholder instance; if an exception is available, you could capture it for more detailed bug reporting.
99-99: Repeats the pattern of creating a plainStacktrace().
Consider providing any caught exception to this constructor for better diagnostic context.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginScreen.kt (3)
99-100: TODO in EMPTY state
There is a placeholder forScreenStateType.EMPTY. Consider providing a user-friendly message or UI to guide users when there’s no data to display.Would you like a proposed UI snippet for this empty state?
103-103: DATA and DATA_WITH_ERROR
Handling these states together is logical. Note that you might unify UI handling of minor errors in a “data-with-error” scenario to avoid duplication of any error UI code.
133-143: Additional ErrorPanelView in DataContent
DisplayingErrorPanelViewinsideDataContentsupports scenarios where data is present but might include minor errors. However, note thatERRORstate also usesErrorPanelView. Consider factoring out into a common composable or conditionally reusing the same block to avoid duplication.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/unlock/UnlockViewModel.kt (2)
83-83: UnusedscreenStateHandler?
screenStateHandlerisn’t referenced elsewhere in this snippet. If it’s no longer needed, consider removing it to keep the codebase lean.
105-105: Remove commented-out code
IfEventProviderImplis permanently obsolete, removing it ensures the code remains clear and free of clutter.app/src/main/java/com/ivanovsky/passnotes/data/entity/OperationError.java (3)
10-10: Consider providingserialVersionUIDfor class implementingSerializable.When a class implements
Serializable, it is often good practice to explicitly provide aserialVersionUIDto maintain compatibility across different versions.public class OperationError implements Serializable { + private static final long serialVersionUID = 1L;
74-75: Minor language refinement.The message is clear, but you might consider making it more explicit, e.g. “A file with an identical name already exists: %s”. Otherwise, this is sufficiently descriptive.
83-193: Consider reducing repetition in multiple factory methods.These factory methods differ only slightly in their parameters, which might be consolidated via a single helper method that accepts the
Type,String?, andThrowable?to cut down on duplication. While explicit methods can help readability, you may benefit from a more DRY approach.app/src/main/kotlin/com/ivanovsky/passnotes/domain/entity/exception/Stacktrace.kt (1)
1-3: CustomStacktraceclass looks good.This class effectively extends
Exception, though in the future you might consider constructors that accept a message or cause for more explanatory stack traces.app/src/main/kotlin/com/ivanovsky/passnotes/extensions/OperationErrorExt.kt (1)
7-35: Handle newly introduced error types or confirm fallback behavior.This extension function provides a user-friendly message for certain error types. Consider adding new cases for
CACHE_ERROR,ERROR_MESSAGE, or others if they need distinct messaging. Otherwise, verify that falling back to the standard message is acceptable.app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/saf/SAFFileSystemSyncProcessor.kt (1)
29-36: Consider adding method-specific error messages.Both
getSyncConflictForFileandprocessmethods return the same generic error messageMESSAGE_INCORRECT_USE_CASE. Consider using more specific error messages to better indicate why these operations are not supported in the SAF file system.For example:
-MESSAGE_INCORRECT_USE_CASE +MESSAGE_SAF_SYNC_CONFLICT_NOT_SUPPORTED // for getSyncConflictForFile +MESSAGE_SAF_SYNC_PROCESS_NOT_SUPPORTED // for processAlso applies to: 38-49
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/BaseScreenViewModel.kt (1)
17-18: Consider prioritizing the suggested refactoring.The TODO comment suggests creating a
BaseCellScreenViewModel. This would improve the separation of concerns by moving cell-related functionality to a dedicated class.Would you like me to help create a new issue to track this refactoring task?
app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/RemoveUsedFileUseCase.kt (1)
37-40: Consider enhancing git-related error handlingThe git-related operations could benefit from similar error handling improvements with stack traces.
if (fsAuthority.type == FSType.GIT) { - removeSshKeyIfNeed(fsAuthority) - removeGitEntryIfNeed(fsAuthority) + val sshKeyResult = removeSshKeyIfNeed(fsAuthority) + if (sshKeyResult.isError) { + return@withContext OperationResult.error( + newDbError("Failed to remove SSH key", Stacktrace()) + ) + } + val gitEntryResult = removeGitEntryIfNeed(fsAuthority) + if (gitEntryResult.isError) { + return@withContext OperationResult.error( + newDbError("Failed to remove Git entry", Stacktrace()) + ) + } }app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/filepicker/FilePickerInteractor.kt (1)
63-75: Consider consolidating file existence handling.While the added existence check is good, there might be redundancy since
openFileForWritealready usesOnConflictStrategy.CANCEL. Consider either:
- Removing the manual existence check and relying on
OnConflictStrategy.CANCEL- Or documenting why both checks are necessary if they serve different purposes
app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/groupEditor/GroupEditorInteractor.kt (2)
48-54: Consider capturing the call site in Stacktrace.While adding Stacktrace improves error reporting, consider capturing the actual call site by passing an exception to Stacktrace constructor for more meaningful stack traces.
- Stacktrace() + Stacktrace(IllegalStateException("Parent UID is null"))
76-82: Enhance error message with more context.The error message could be more descriptive by including the attempted title in the error message.
- newGenericError( - resourceProvider.getString(R.string.group_with_this_name_is_already_exist), - Stacktrace() + newGenericError( + resourceProvider.getString(R.string.group_with_this_name_is_already_exist) + ": " + entity.title, + Stacktrace(IllegalStateException("Group title already exists: ${entity.title}"))app/src/main/kotlin/com/ivanovsky/passnotes/domain/FileHelper.kt (3)
42-48: Add specific exception type for file access errors.Enhance error context by using a specific exception type for file access errors.
- Stacktrace() + Stacktrace(SecurityException("Failed to access private storage: ${context.filesDir}"))
53-58: Improve error handling in generateDestinationDirectoryForSharedFile.The method has two error cases that could benefit from more specific exceptions and error messages.
- Stacktrace() + Stacktrace(SecurityException("Failed to access shared files directory: ${context.cacheDir}")) - Stacktrace() + Stacktrace(IOException("Failed to create directory: $path"))Also applies to: 62-69
78-83: Enhance error context for private file access.Add more context to the error when accessing private files.
- Stacktrace() + Stacktrace(SecurityException("Failed to access private storage for file: $name"))app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/TemplateDaoImpl.kt (1)
86-92: Enhance error message with template group context.Add more context to the error when a template group already exists.
- Stacktrace() + Stacktrace(IllegalStateException("Template group already exists: $TEMPLATE_GROUP_NAME"))app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/noteEditor/NoteEditorInteractor.kt (2)
114-123: Enhance duplicate file error handling.Add more context to the error when a file is already added.
- Stacktrace() + Stacktrace(IllegalStateException("Duplicate file detected: ${file.name}, hash: $hash"))
84-139: Consider additional error cases in createAttachment.The method could benefit from additional error handling:
- File size limits
- File type validation
- Content validation
Would you like me to provide an implementation that includes these additional checks?
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/webdav/WebDavClientV2.kt (1)
46-51: Consider adding more context to the error message.The error message could be more descriptive by including the directory path.
- return OperationResult.error( - newFileAccessError( - MESSAGE_FILE_IS_NOT_A_DIRECTORY, - Stacktrace() - ) - ) + return OperationResult.error( + newFileAccessError( + String.format("%s: %s", MESSAGE_FILE_IS_NOT_A_DIRECTORY, dir.path), + Stacktrace() + ) + )app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/git/GitRepository.kt (1)
168-168: Enhance error messages for git operations.The error messages for pull and push operations could be more descriptive by including the error details from the git commands.
- return OperationResult.error(newRemoteApiError(ERROR_FAILED_TO_PULL, Stacktrace())) + return OperationResult.error(newRemoteApiError( + String.format("%s: %s", ERROR_FAILED_TO_PULL, pull.rebaseResult?.status), + Stacktrace() + ))- return OperationResult.error(newRemoteApiError(ERROR_FAILED_TO_PUSH, Stacktrace())) + return OperationResult.error(newRemoteApiError( + String.format("%s: %s", ERROR_FAILED_TO_PUSH, push.map { it.remoteUpdates.map { it.status } }), + Stacktrace() + ))Also applies to: 226-226
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/kotpass/KotpassGroupDao.kt (1)
109-114: Enhance error messages for group operations.The error messages could be more descriptive by including the group details.
- return@withLock OperationResult.error( - newDbError( - MESSAGE_PARENT_UID_IS_NULL, - Stacktrace() - ) - ) + return@withLock OperationResult.error( + newDbError( + String.format("%s for group: %s", MESSAGE_PARENT_UID_IS_NULL, entity.title), + Stacktrace() + ) + )- return@withLock OperationResult.error( - newDbError( - MESSAGE_FAILED_TO_MOVE_GROUP_INSIDE_ITS_OWN_TREE, - Stacktrace() - ) - ) + return@withLock OperationResult.error( + newDbError( + String.format("%s: %s -> %s", MESSAGE_FAILED_TO_MOVE_GROUP_INSIDE_ITS_OWN_TREE, entity.uid, entity.parentUid), + Stacktrace() + ) + )Also applies to: 308-313
app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/debugmenu/DebugMenuInteractor.kt (1)
98-99: Enhance error messages for file operations.The error messages could be more descriptive by including the file paths.
- result.error = - newGenericIOError(MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE, Stacktrace()) + result.error = + newGenericIOError(String.format("%s: %s", MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE, outFile.path), Stacktrace())- result.error = - newGenericIOError(MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE, Stacktrace()) + result.error = + newGenericIOError(String.format("%s: Failed to create output file", MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE), Stacktrace())- result.error = - newGenericIOError(MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE, Stacktrace()) + result.error = + newGenericIOError(String.format("%s: %s", MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE, dbFile.path), Stacktrace())- result.error = - newGenericIOError(MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE, Stacktrace()) + result.error = + newGenericIOError(String.format("%s: Failed to create database file", MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE), Stacktrace())- result.error = - newGenericIOError(MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE, Stacktrace()) + result.error = + newGenericIOError(String.format("%s: %s", MESSAGE_FAILED_TO_ACCESS_TO_PRIVATE_STORAGE, inFile.path), Stacktrace())Also applies to: 102-103, 190-191, 197-198, 248-249
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/git/GitClient.kt (1)
441-443: LGTM! Error handling improvements look good.The addition of
Stacktrace()to error factory methods enhances debugging capabilities by providing stack trace information. The changes are consistent across all error handling methods.Consider extracting the error creation logic into a separate helper class to reduce code duplication and centralize error handling. For example:
+private class GitErrorFactory { + companion object { + fun invalidCredentials() = + newGenericError(MESSAGE_INCORRECT_FILE_SYSTEM_CREDENTIALS, Stacktrace()) + + fun invalidGitEntry(entry: String) = + newGenericError(String.format(GENERIC_INVALID_DATABASE_ENTRY, entry), Stacktrace()) + + // ... other error factory methods + } +}Also applies to: 445-452, 455-462, 465-472, 475-482
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/kotpass/KotpassDatabase.kt (1)
134-136: LGTM! Error handling improvements look good.The addition of
Stacktrace()to error factory methods enhances debugging capabilities by providing stack trace information. The changes are consistent across all error handling methods.Consider extracting the error creation logic into a separate helper class to reduce code duplication and centralize error handling, similar to the suggestion for GitClient.kt. For example:
+private class KotpassErrorFactory { + companion object { + fun groupNotFound() = + newDbError(MESSAGE_FAILED_TO_FIND_GROUP, Stacktrace()) + + fun entityNotFound(entityName: String, uid: UUID) = + newDbError(String.format(GENERIC_MESSAGE_FAILED_TO_FIND_ENTITY_BY_UID, + entityName, uid), Stacktrace()) + + // ... other error factory methods + } +}Also applies to: 212-217, 229-234, 249-251, 317-322, 406-411
app/src/main/res/layout/dialog_change_password.xml (3)
19-29: Duplicate Binding in ErrorPanelView
The ErrorPanelView now receives both a plainstateattribute and anapp:screenStateattribute (both bound to@{viewModel.screenState}) along with the newapp:screenVisibilityHandlerbinding. This duplication may be unintentional unless the view is explicitly designed to accept two separate bindings for state. Please verify if both attributes are required or if one should be removed to avoid confusion.
31-49: Consistent Update of MaterialEditText Bindings
For the MaterialEditText components, the update replaces the older state handling (presumably via ascreenStateHandler) with the newapp:screenVisibilityHandlerattribute while still bindingapp:screenStateto@{viewModel.screenState}. The updates appear consistent with the refactor; however, consider reviewing whether the dual binding of state—via bothapp:screenStateand the newstateattribute used in other views—is necessary.
114-125: ScreenStateView Attribute Duplication
Similar to the ErrorPanelView, the ScreenStateView now includes a directstateattribute (line 115) as well as anapp:screenStateattribute (line 124) both bound to the same view model property. Please double-check if both are needed or if consolidating to a single binding is possible to simplify the layout.app/src/main/res/layout/debug_menu_fragment.xml (2)
24-32: ErrorPanelView Updated Correctly with Unified Visibility Handler
The ErrorPanelView here now uses both the newstateattribute (line 26) and bindsapp:screenVisibilityHandlerto@{viewModel.screenVisibilityHandler}(line 32). This matches the pattern observed in dialog_change_password.xml. However, as with the previous file, the presence of bothstateand the previously usedapp:screenStateattribute (line 31) could be redundant. Verify whether the dual binding is intentional.
34-41: ScreenStateView Binding Consistency
The ScreenStateView in this layout similarly includes both a direct binding viastate(line 35) and anapp:screenStatebinding (line 40), along with the newapp:screenVisibilityHandlerattached to@{viewModel.screenVisibilityHandler}. The use is consistent with the new pattern here, but please confirm that maintaining both state bindings is necessary and that there is no overlap betweenstateandapp:screenState.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/BaseComposeDialog.kt (1)
15-42: Consider adding code documentation and sample usage.This abstract class provides a solid framework for Compose-based dialogs. Including KDoc annotations, lifecycle commentary, and usage examples would help future contributors quickly understand how to implement specific dialog behavior by overriding
RenderDialog().app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialogArgs.kt (1)
6-8: Consider using Parcelable or Kotlin's serialization instead of Java'sSerializable.While
Serializableworks, it's generally less performant and flexible compared toParcelableor kotlinx.serialization. Switching to a more Android-friendly or Kotlin-native serialization format can help avoid potential runtime issues and improve efficiency.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialog.kt (2)
14-19: Add null-safety or fallback behavior for missing arguments.
requireArgument(ARGUMENTS)throws an exception ifargumentsis absent. Consider offering a gentle fallback or error flow to enhance stability and user experience.
21-26: Leverageby viewModels()for simpler ViewModel instantiation.Using
by viewModels()in Fragment-based code provides more concise syntax and enforces best practices for lifecycle-aware ViewModels. Evaluate if this suits your design.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialogViewModel.kt (1)
28-46: Consider adding error handling for clipboard operations.The clipboard operation could fail, but there's no error handling or user feedback mechanism in place.
fun onCopyButtonClicked() { val message = args.error.formatReadableMessage(resourceProvider) val stacktrace = args.error.throwable?.stackTraceToString() ?: StringUtils.EMPTY if (message.isEmpty() && stacktrace.isEmpty()) { return } val fullText = when { stacktrace.isEmpty() -> message message.isEmpty() -> stacktrace else -> message + LINE_BREAK + stacktrace } - clipboardInteractor.copy( - text = fullText, - isProtected = false - ) + try { + clipboardInteractor.copy( + text = fullText, + isProtected = false + ) + // TODO: Show success feedback to user + } catch (e: Exception) { + // TODO: Show error feedback to user + Timber.e(e, "Failed to copy to clipboard") + } }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialogScreen.kt (1)
63-108: Consider adding content description for accessibility.The clickable Box that toggles the stack trace visibility should have a content description for screen readers.
Box( contentAlignment = Alignment.Center, modifier = Modifier .padding(top = ElementMargin) .fillMaxWidth() .defaultMinSize(minHeight = 48.dp) - .clickable(onClick = onToggleState) + .clickable( + onClick = onToggleState, + onClickLabel = if (state.isStacktraceCollapsed) + stringResource(R.string.show_stacktrace) + else stringResource(R.string.hide_stacktrace) + ) )app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/widget/ErrorPanelView.kt (1)
21-22: Consolidate constructors.Consider unifying these constructors so the second constructor calls the first one, reducing duplication:
- constructor(context: Context, attrs: AttributeSet) : super(context, attrs) - constructor(context: Context) : super(context, null) + constructor(context: Context, attrs: AttributeSet? = null) : super(context, attrs)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groupEditor/GroupEditorViewModel.kt (4)
33-35: Remove repeated loading state, if unnecessary.Currently, the initial state is set to loading here, and also set to loading again in
loadData()(line 59). If callingloadData()is guaranteed at startup, you might avoid duplication by omitting one of these calls to reduce boilerplate.
37-37: Update variable name for clarity and consistency.The field is called
screenStateHandleryet instantiatesDefaultScreenVisibilityHandler. Consider renaming it to something likescreenVisibilityHandlerto match the new visibility-based approach.- val screenStateHandler = DefaultScreenVisibilityHandler() + val screenVisibilityHandler = DefaultScreenVisibilityHandler()
137-137: Clarify usage ofsetErrorPanelState.Here,
setErrorPanelStateis used instead ofsetErrorState. Consider unifying the error handling methods to simplify the code and reduce confusion.
171-171: Use a consistent error handling method.Similarly to the
createNewGroupflow, ensure consistency betweensetErrorPanelStateandsetErrorState. Maintaining one primary function can help standardize error handling across the codebase.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginViewModel.kt (2)
45-45: Consider exposing a read-only StateFlow.
It's common practice in Kotlin to keep theMutableStateFlowprivate and expose only aStateFlow<ServerLoginState>to avoid external mutations of the ViewModel state.private val _state = MutableStateFlow(createInitialState()) -fun onUrlChanged(url: String) { +val state: StateFlow<ServerLoginState> = _state // Then in the code, replace direct references to `state` with `_state`.
242-252: Simplify validation return logic.
Currently, the method setsurlErrorif blank but then returns based onurl.isNotBlank(). For clarity, consider an explicit short-circuit:private fun validateFields(): Boolean { if (state.value.url.isBlank()) { setScreenState( state.value.copy( urlError = resourceProvider.getString(R.string.empty_field) ) ) - } - - return state.value.url.isNotBlank() + return false + } + return true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
app/build.gradle(3 hunks)app/src/main/java/com/ivanovsky/passnotes/data/entity/OperationError.java(3 hunks)app/src/main/java/com/ivanovsky/passnotes/data/repository/file/remote/RemoteFileSyncProcessor.java(10 hunks)app/src/main/java/com/ivanovsky/passnotes/data/repository/file/remote/RemoteFileSystemProvider.java(16 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/crypto/biometric/BiometricCipherProvider.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/fake/FakeFileSystemProvider.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/fake/FakeFileSystemSyncProcessor.kt(6 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/git/GitClient.kt(5 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/git/GitRepository.kt(7 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemProvider.kt(8 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemSyncProcessor.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/saf/SAFFileSystemProvider.kt(6 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/saf/SAFFileSystemSyncProcessor.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/saf/SAFHelper.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/webdav/WebDavClientV2.kt(7 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/webdav/WebDavNetworkLayer.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/KeepassDatabaseRepository.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/TemplateDaoImpl.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/kotpass/KotpassDatabase.kt(7 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/kotpass/KotpassGroupDao.kt(4 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/kotpass/KotpassNoteDao.kt(4 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/FileHelper.kt(5 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/entity/exception/Stacktrace.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/ErrorInteractor.kt(0 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/debugmenu/DebugMenuInteractor.kt(10 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/filepicker/FilePickerInteractor.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/groupEditor/GroupEditorInteractor.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/newdb/NewDatabaseInteractor.kt(5 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/noteEditor/NoteEditorInteractor.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/settings/app/AppSettingsInteractor.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/unlock/UnlockInteractor.kt(5 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/test/TestDataBroadcastReceiver.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/test/TestDataParser.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/test/biometric/DebugBiometricInteractorImpl.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/test/usecases/SetupFakeFileUseCase.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/DecodePasswordWithBiometricUseCase.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/EncodePasswordWithBiometricUseCase.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/FindParentGroupsUseCase.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/GetDatabaseUseCase.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/GetUsedFileUseCase.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/RemoveUsedFileUseCase.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/extensions/OperationErrorExt.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/injection/modules/CoreModule.kt(0 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/injection/modules/UiModule.kt(4 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/about/AboutScreen.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/BaseScreenViewModel.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/DefaultScreenStateHandler.kt(0 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/DefaultScreenVisibilityHandler.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/ScreenState.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/ScreenVisibilityHandler.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/ViewModelFactory.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/binding/BindingAdapters.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/Dimens.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/ErrorState.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/Preview.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/TextField.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/cells/ui/InfoCell.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/BaseComposeDialog.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/reportErrorDialog/ReportErrorDialog.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialogArgs.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialogScreen.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialogViewModel.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/model/ReportErrorState.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/resolveConflict/ResolveConflictDialogViewModel.kt(4 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/extensions/BundleExt.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/widget/ErrorPanelView.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/widget/ScreenStateView.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/debugmenu/DebugMenuViewModel.kt(20 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/diffViewer/DiffViewerInteractor.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/diffViewer/DiffViewerViewModel.kt(5 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/enterDbCredentials/EnterDbCredentialsViewModel.kt(5 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/filepicker/FilePickerViewModel.kt(12 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groupEditor/GroupEditorViewModel.kt(5 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/GroupsFragment.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/GroupsViewModel.kt(15 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/history/HistoryViewModel.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/newdb/NewDatabaseViewModel.kt(5 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/note/NoteViewModel.kt(8 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/noteEditor/NoteEditorViewModel.kt(16 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginFragment.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginScreen.kt(7 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginViewModel.kt(10 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/model/ServerLoginIntent.kt(0 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/model/ServerLoginState.kt(1 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/service/DatabaseLockBroadcastReceiver.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/app/AppSettingsViewModel.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/database/DatabaseSettingsViewModel.kt(3 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/database/changePassword/ChangePasswordDialogViewModel.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/database/changePassword/ChangePasswordScreenVisibilityHandler.kt(2 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/storagelist/StorageListViewModel.kt(11 hunks)app/src/main/kotlin/com/ivanovsky/passnotes/presentation/unlock/UnlockViewModel.kt(12 hunks)app/src/main/res/layout/core_compose_cell.xml(1 hunks)app/src/main/res/layout/core_compose_dialog.xml(1 hunks)app/src/main/res/layout/debug_menu_fragment.xml(2 hunks)app/src/main/res/layout/dialog_change_password.xml(7 hunks)app/src/main/res/layout/dialog_resolve_conflict.xml(4 hunks)app/src/main/res/layout/diff_viewer_fragment.xml(2 hunks)app/src/main/res/layout/enter_db_credentials_fragment.xml(2 hunks)app/src/main/res/layout/file_picker_fragment.xml(3 hunks)app/src/main/res/layout/group_editor_fragment.xml(4 hunks)app/src/main/res/layout/groups_fragment.xml(3 hunks)app/src/main/res/layout/new_database_fragment.xml(1 hunks)app/src/main/res/layout/note_editor_fragment.xml(3 hunks)app/src/main/res/layout/note_fragment.xml(4 hunks)app/src/main/res/layout/storage_list_fragment.xml(2 hunks)app/src/main/res/layout/unlock_fragment.xml(4 hunks)
⛔ Files not processed due to max files limit (7)
- app/src/main/res/layout/view_error_panel.xml
- app/src/main/res/layout/view_screen_state.xml
- app/src/main/res/values/dimens.xml
- app/src/main/res/values/strings.xml
- app/src/main/res/values/styles.xml
- app/src/main/res/values/untranslatable-strings.xml
- version.properties
💤 Files with no reviewable changes (4)
- app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/ErrorInteractor.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/injection/modules/CoreModule.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/DefaultScreenStateHandler.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/model/ServerLoginIntent.kt
✅ Files skipped from review due to trivial changes (7)
- app/src/main/res/layout/core_compose_dialog.xml
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/model/ReportErrorState.kt
- app/src/main/res/layout/core_compose_cell.xml
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/ErrorState.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/cells/ui/InfoCell.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/ScreenVisibilityHandler.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/Dimens.kt
👮 Files not reviewed due to content moderation or server errors (4)
- app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/fake/FakeFileSystemSyncProcessor.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/KeepassDatabaseRepository.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/saf/SAFFileSystemProvider.kt
- app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemProvider.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (304)
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/binding/BindingAdapters.kt (3)
35-35: LGTM!The import change from
ScreenStateHandlertoScreenVisibilityHandleraligns with the visibility management refactor.
55-64: LGTM!The method rename and parameter update improve clarity by explicitly indicating the visibility handling responsibility. The implementation is simplified to use a single
applyScreenStatecall.
235-235: LGTM!The property rename from
buttonClickListenertoactionButtonClickListeneris more descriptive and better indicates the purpose of the listener.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/about/AboutScreen.kt (1)
56-56: LGTM! Good choice redirecting users to the issues page.The change from
feedback_urltoissues_urlaligns well with the PR's objective of improving error handling by directing users to a more appropriate channel for reporting issues.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginFragment.kt (1)
128-130: LGTM! Improved menu action handling.The refactoring from intent-based to direct method calls simplifies the code, making it more maintainable and easier to follow. The type-safe implementation maintains clear separation of concerns while reducing boilerplate.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/database/changePassword/ChangePasswordScreenVisibilityHandler.kt (2)
12-12: LGTM! Clear and consistent class naming.The rename from
ChangePasswordScreenStateHandlertoChangePasswordScreenVisibilityHandlerbetter reflects its responsibility of managing view visibility rather than generic state handling.
14-33: LGTM! Well-structured visibility handling with clear documentation.The implementation demonstrates good practices:
- Clear documentation explaining the visibility strategy
- Explicit handling of different view types
- Smart use of
isInvisibleto maintain dialog dimensionsapp/src/main/kotlin/com/ivanovsky/passnotes/presentation/history/HistoryViewModel.kt (2)
13-13: LGTM!The import statement is correctly added to support the new error handling approach.
144-144: LGTM! Simplified error handling.The change improves the code by:
- Simplifying error handling through direct message formatting.
- Reducing dependencies by removing the intermediary
ErrorInteractor.- Maintaining consistent error handling across the codebase.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/database/DatabaseSettingsViewModel.kt (4)
9-9: LGTM! Import changes align with error handling refactor.The new imports for
ResourceProviderandformatReadableMessageextension function support the transition fromErrorInteractorto the new error handling approach.Also applies to: 11-11
18-18: LGTM! Constructor parameter updated to use ResourceProvider.The replacement of
ErrorInteractorwithResourceProvideraligns with the PR's error handling refactor.
41-41: LGTM! Error handling updated to use new formatting approach.The error handling now uses the
formatReadableMessageextension function withresourceProvider, making the code more concise while maintaining functionality.
64-64: LGTM! Error handling consistently uses new formatting approach.The error handling in
onRecycleBinEnabledChangedfollows the same pattern as instart(), maintaining consistency across the ViewModel.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/extensions/BundleExt.kt (1)
8-18: LGTM! Clean and efficient implementation.The function correctly handles API level differences for serializable retrieval, using the type-safe approach for Android 13+ while maintaining backward compatibility.
app/src/main/kotlin/com/ivanovsky/passnotes/domain/test/biometric/DebugBiometricInteractorImpl.kt (3)
6-6: LGTM! Import changes align with error handling refactor.The import changes effectively support the new error handling approach by importing the specific error factory method and the Stacktrace class.
Also applies to: 11-11
29-29: LGTM! Error handling updated to include stack trace information.The change enhances debugging capabilities by including stack trace information in the error.
39-39: LGTM! Consistent error handling approach.The error handling approach is consistent with getCipherForEncryption, maintaining a uniform pattern across the class.
app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/FindParentGroupsUseCase.kt (2)
5-6: LGTM!The imports are correctly organized and necessary for the error handling refactor.
Also applies to: 9-9
50-56: LGTM!The error handling follows the new pattern correctly. The same optimization suggested above about reusing the Stacktrace instance applies here.
app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/unlock/UnlockInteractor.kt (4)
32-32: LGTM!The import is correctly added to support enhanced error handling with stack traces.
144-149: LGTM!The error handling is enhanced with stack trace information while maintaining the existing timeout logic.
249-251: LGTM!The error handling is enhanced with stack trace information while maintaining proper error message formatting.
272-276: LGTM!The error handling is enhanced with stack trace information while maintaining consistent error handling patterns.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/GroupsFragment.kt (2)
184-189: LGTM! Property rename maintains functionality.The renaming of
cellViewModelstocellsimproves naming conciseness while maintaining the same functionality.
231-233: LGTM! Streamlined error handling implementation.The new error handling approach using
showMessageDialogEventwith direct error formatting is cleaner and more maintainable.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/diffViewer/DiffViewerInteractor.kt (2)
14-14: LGTM!The import is correctly added to support the enhanced error handling with stack trace information.
79-84: LGTM!The error handling enhancement with stack trace information is well implemented. This code would also benefit from the suggested refactoring to use a shared error creation method.
app/src/main/kotlin/com/ivanovsky/passnotes/domain/test/TestDataBroadcastReceiver.kt (2)
9-10: LGTM! Clean transition to the new error handling approach.The replacement of ErrorInteractor with ResourceProvider and the addition of formatReadableMessage extension maintains clean dependency injection while aligning with the new error handling pattern.
Also applies to: 18-18
26-26: LGTM! Consistent error handling implementation.The error handling has been cleanly updated to use the new formatReadableMessage pattern while maintaining the existing logging and user notification behavior.
Also applies to: 35-35
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/noteEditor/NoteEditorViewModel.kt (3)
35-35: LGTM! Good architectural improvement.The switch from
DefaultScreenStateHandlertoDefaultScreenVisibilityHandlerimproves separation of concerns and aligns with the broader refactor across view models.Also applies to: 91-91
822-825: LGTM! Well-implemented screen state management.The
setScreenStateoverride properly encapsulates the visibility logic and maintains a clean separation of concerns.
136-136: LGTM! Improved method naming.The rename from
setCellElementstosetCellViewModelsis more descriptive and consistently applied across all usages, making the code more self-documenting.Also applies to: 300-300, 307-307, 334-334, 355-355, 417-417, 445-445, 483-483, 516-516
app/src/main/res/layout/note_fragment.xml (5)
27-28: Update Attribute Naming for Visibility Handling in FrameLayout
The replacement ofapp:screenStateHandlerwithapp:screenVisibilityHandlerin the FrameLayout is applied correctly. Please verify that the ViewModel provides the proper property (currently bound asviewModel.screenStateHandler) and that this new attribute name is consistently supported by the custom attribute definitions.
38-46: Refactor ErrorPanelView for Improved Error Display
In ErrorPanelView, the addition of thestate="@{viewModel.screenState}"attribute and the new background attribute (android:background="?kpErrorBackgroundColor") ensures that error formatting now aligns with the new design and visibility handling strategy. The update fromapp:screenStateHandlertoapp:screenVisibilityHandleris also correctly applied. Please confirm that the color attribute (?kpErrorBackgroundColor) is defined in your theme resources to avoid runtime issues.
48-53: Standardize ScreenStateView Attributes
The changes in ScreenStateView introducestate="@{viewModel.screenState}"and update the visibility handler attribute fromapp:screenStateHandlertoapp:screenVisibilityHandler. This enhances consistency with the overall refactor. Please verify that the view model’sscreenStateandscreenStateHandlerproperties deliver the expected values for proper UI behavior.
55-64: Update RecyclerView with New Visibility Handler
The RecyclerView now usesapp:screenVisibilityHandler(line 64) instead of the old handler attribute. This change aligns with the new unified error and state display approach. Ensure that theviewModel.screenStateHandlerprovides the correct binding data and that the RecyclerView’s visibility behavior matches the intended design.
66-76: Consistent Visibility Handler in CardView
The CardView has been updated to useapp:screenVisibilityHandler(line 76) in place of the previous attribute. This update supports a consistent handling of screen visibility across components. Please verify that no nested view behaviors are adversely affected by this change.app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemSyncProcessor.kt (2)
15-15: LGTM!The import is correctly added to support the enhanced error handling with stack traces.
58-63: LGTM!The error handling is consistently implemented with stack trace support, matching the pattern in
getSyncConflictForFile.The same optimization suggested above would apply here:
override fun process( file: FileDescriptor, syncStrategy: SyncStrategy, resolutionStrategy: ConflictResolutionStrategy? ): OperationResult<FileDescriptor> { - return OperationResult.error( - newGenericError( - MESSAGE_INCORRECT_USE_CASE, - Stacktrace() - ) - ) + return OperationResult.error(incorrectUseCaseError) }app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/app/AppSettingsViewModel.kt (3)
14-14: LGTM!The import of
formatReadableMessageextension function aligns with the new error handling approach.
99-99: LGTM!The error handling has been simplified by using the extension function while maintaining proper localization through
resourceProvider.
115-115: LGTM!The error handling has been simplified consistently with the rest of the codebase.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/filepicker/FilePickerViewModel.kt (4)
54-54: LGTM: Screen state handler updated to use visibility-based approach.The change from
DefaultScreenStateHandlertoDefaultScreenVisibilityHandleraligns with the refactoring to use a unified visibility handler.
96-101: LGTM: Improved error handling with stack traces.Error handling has been enhanced by consistently including stack traces across all error messages. This will improve debugging capabilities and error tracing.
Also applies to: 114-119, 189-194, 277-277, 303-303
377-380: LGTM: Proper method override implementation.The
setScreenStateoverride correctly calls the super implementation while maintaining its specific functionality for the done button visibility.
142-145: LGTM: Improved method naming for better clarity.The rename from
selectFiletoselectFileAndExitbetter describes the method's behavior, making the code more maintainable.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/DefaultScreenVisibilityHandler.kt (2)
1-14: LGTM! Well-organized imports.The imports are clean, relevant, and properly organized.
15-15: LGTM! Well-designed class declaration.The class is appropriately marked as
opento allow for custom implementations while providing a default behavior.app/src/main/java/com/ivanovsky/passnotes/data/repository/file/remote/RemoteFileSyncProcessor.java (5)
39-39: LGTM!The Stacktrace import is correctly added and aligns with the error handling refactor.
183-184: LGTM!The error handling has been consistently updated to include stack traces while maintaining appropriate error types and messages.
Also applies to: 197-198, 208-209
236-237: LGTM!The error handling in the process method has been consistently updated to include stack traces while maintaining appropriate error types.
Also applies to: 287-288, 293-294
314-315: LGTM!The error handling in the uploadLocalFile method has been consistently updated to include stack traces with appropriate error types.
Also applies to: 334-334, 340-341
418-419: LGTM!The error handling in the downloadFile method has been consistently updated to include stack traces with appropriate error types.
Also applies to: 427-428, 434-435
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/diffViewer/DiffViewerViewModel.kt (4)
47-49: LGTM! Good improvement in state initialization.Setting the initial state to loading in the constructor provides a clear initial UI state.
256-256: LGTM! Improved error handling consistency.The change to use
setErrorStateconsistently across all error scenarios improves code maintainability and provides a uniform error handling approach.Also applies to: 262-262, 302-302
338-338: LGTM! More precise method naming.Renaming from
setCellElementstosetCellViewModelsbetter reflects the type of data being set and aligns with view model terminology.
57-57:Details
✅ Verification successful
Verify consistent usage of DefaultScreenVisibilityHandler.
The change from DefaultScreenStateHandler to DefaultScreenVisibilityHandler appears to be part of a broader refactor.
Let's verify the consistent usage across the codebase:
🏁 Script executed:
#!/bin/bash # Search for any remaining usages of DefaultScreenStateHandler rg "DefaultScreenStateHandler" # Search for new DefaultScreenVisibilityHandler usage pattern rg "DefaultScreenVisibilityHandler"Length of output: 4586
Consistent usage verified: The refactor to replace
DefaultScreenStateHandlerwithDefaultScreenVisibilityHandleris consistently applied across the codebase. All occurrences now use the new class, and no references to the old name remain.app/src/main/res/layout/group_editor_fragment.xml (4)
29-36: ScreenStateView RefactoringThe ScreenStateView now includes a
stateattribute and a newapp:screenVisibilityHandlerattribute. This change helps align its behavior with the unified UI visibility handling strategy. Ensure the ViewModel binding—still referencingviewModel.screenStateHandler—is correct and consistent with the new naming conventions adopted across the codebase.
38-51: MaterialEditText Update for Visibility HandlingMaterialEditText has been updated with the
app:screenVisibilityHandlerattribute. This brings its error/visibility handling inline with the new approach. As with the other components, double-check that binding toviewModel.screenStateHandleris intended or whether the ViewModel property should also be renamed to match the new attribute name.
53-67: First MaterialSpinner ConsistencyThe first MaterialSpinner now gains the
app:screenVisibilityHandlerattribute to ensure consistent UI behavior across components. Please confirm that usingviewModel.screenStateHandleras a binding value is consistent with your overall refactoring if the intention is to use a property named in line withscreenVisibilityHandler.
69-83: Second MaterialSpinner UpdateSimilarly, the second MaterialSpinner now also utilizes the
app:screenVisibilityHandlerattribute. This improves the consistency of the UI’s state management. As before, verify that the reference toviewModel.screenStateHandleris aligned with the new handling convention, or update it if the ViewModel property has been renamed.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/storagelist/StorageListViewModel.kt (2)
19-19: LGTM! Improved error handling structure.The changes align well with the refactoring objectives, moving from
ErrorInteractorto a more direct error handling approach withOperationErrorandStacktrace. The switch toDefaultScreenVisibilityHandlerprovides better visibility management.Also applies to: 24-24, 30-30, 59-59
192-198: Verify screen state handling during navigation.There's a potential race condition in the navigation flow where the screen state is set to loading twice in quick succession:
- Line 192: Before navigation
- Line 197: After navigation result
This might cause UI flickering or inconsistent states.
Consider either:
- Removing the second loading state since the navigation already shows loading
- Adding a delay before showing the second loading state
setScreenState(ScreenState.loading()) isReloadOnStart = true router.setResultListener(ServerLoginScreen.RESULT_KEY) { file -> if (file is FileDescriptor) { isReloadOnStart = false - setScreenState(ScreenState.loading()) if (file.isDirectory) { navigateToFilePicker(app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/model/ServerLoginState.kt (1)
4-4: Ensure correct usage of imported ScreenStateThe import for
ScreenStatelooks valid, but please confirm it's the intended class from the correct package.Would you like me to run a shell script to confirm that the
ScreenStateclass is correctly located incom.ivanovsky.passnotes.presentation.coreand is not duplicated elsewhere in the codebase?app/src/main/kotlin/com/ivanovsky/passnotes/injection/modules/UiModule.kt (3)
34-35: LGTM!The new imports for error reporting dialog components align with the PR's objective of refactoring error handling.
390-396: LGTM!The addition of the
ReportErrorDialogViewModelfactory method is consistent with the PR's objective of refactoring error handling. The implementation follows the same pattern as other dialog ViewModels in the module.
329-329:Details
✅ Verification successful
Verify the removal of dependencies from ViewModels.
The removal of parameters from multiple ViewModel factory methods suggests that error handling dependencies have been removed. This aligns with the PR's objective of refactoring error handling, but we should verify that these ViewModels can still handle errors appropriately.
Run this script to check the updated ViewModel constructors and their error handling capabilities:
Also applies to: 331-331, 343-343, 344-344, 364-364
🏁 Script executed:
#!/bin/bash # Description: Verify that removed dependencies haven't broken error handling in ViewModels # Check AppSettingsViewModel constructor ast-grep --pattern 'class AppSettingsViewModel($$$) { $$$ }' # Check ChangePasswordDialogViewModel constructor ast-grep --pattern 'class ChangePasswordDialogViewModel($$$) { $$$ }' # Check DebugMenuViewModel constructor ast-grep --pattern 'class DebugMenuViewModel($$$) { $$$ }' # Check NewDatabaseViewModel constructor ast-grep --pattern 'class NewDatabaseViewModel($$$) { $$$ }' # Check DiffViewerViewModel constructor ast-grep --pattern 'class DiffViewerViewModel($$$) { $$$ }' # Search for error handling methods in these ViewModels rg -A 5 'fun.*Error|fun.*Exception'Length of output: 46272
Refactor Verified: Error handling remains intact in the updated ViewModels.
After verifying with the provided scripts, there’s no indication that the removal of error handling dependencies has broken any functionality. The searches confirm that error handling mechanisms (e.g., handling for exceptions and error states) are still implemented across the codebase. The refactor appears safe as the centralized error handling functions (used in related classes and utility methods) continue to be invoked correctly.
- In
app/src/main/kotlin/com/ivanovsky/passnotes/injection/modules/UiModule.kt(line 329), the updatedviewModel { AppSettingsViewModel(get(), get(), get(), get(), get(), get()) }definition is consistent with the overall error management strategy.- Verification for ViewModels like
ChangePasswordDialogViewModel,DebugMenuViewModel, etc., showed no missing implementations for error handling.app/src/main/res/layout/groups_fragment.xml (6)
53-53: New State Binding for ErrorPanelView
The addition ofstate="@{viewModel.screenState}"directly binds the error state to the view model. This improves clarity by making the view’s state explicit and easier to manage.
56-56: Error Background Theme Update
The updated attributeandroid:background="?kpErrorBackgroundColor"ensures that the error panel reflects the new theme styling. Please verify that the?kpErrorBackgroundColorresource is defined in your theme resources to avoid runtime issues.
59-59: Adoption of Unified Visibility Handler in ErrorPanelView
Replacing the old handler withapp:screenVisibilityHandler="@{viewModel.screenStateHandler}"aligns error display with the new UI refactor. Confirm thatviewModel.screenStateHandleris implemented to support the intended visibility logic.
62-66: ScreenStateView: Consistent State Binding and Visibility Handling
The addition ofstate="@{viewModel.screenState}"and the update toapp:screenVisibilityHandler="@{viewModel.screenStateHandler}"in theScreenStateViewcomponent ensure a consistent approach to state and visibility management across the UI.
87-87: RecyclerView Visibility Handler Update
Updating the RecyclerView withapp:screenVisibilityHandler="@{viewModel.screenStateHandler}"contributes to a uniform strategy for managing component visibility based on the view model’s state.
110-110: FrameLayout Visibility Handler Consistency
The inclusion ofapp:screenVisibilityHandler="@{viewModel.screenStateHandler}"in the FrameLayout maintains consistency with the newly introduced visibility handling pattern. Ensure that any associated visibility logic in the view model aligns with these changes.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/resolveConflict/ResolveConflictDialogViewModel.kt (1)
30-34: Confirm consistent usage ofBaseScreenViewModelinitialization
Switching toBaseScreenViewModelwith an initial loading state aligns with the new refactored architecture. However, ensure all references and overrides (e.g.,onStart(),onStop(), etc.) are consistently updated across this view model, and confirm the initial loading state does not conflict with the user experience.app/src/main/kotlin/com/ivanovsky/passnotes/domain/test/usecases/SetupFakeFileUseCase.kt (2)
17-17: LGTM!The import is correctly placed and necessary for the enhanced error handling.
99-107:Details
✅ Verification successful
LGTM! Verify consistent usage across the codebase.
The addition of
Stacktrace()aligns with the broader error handling refactor. Let's verify that similar changes have been made consistently across the codebase.
🏁 Script executed:
#!/bin/bash # Description: Verify consistent usage of Stacktrace in error factory methods. # Test: Search for all usages of newFileNotFoundError to ensure they include Stacktrace. rg -A 5 $'newFileNotFoundError'Length of output: 23674
LGTM! The addition of the inline
Stacktrace()is applied consistently.The update in
SetupFakeFileUseCase.ktto include theStacktrace()parameter aligns with the broader error handling refactor. Verification via the grep search confirms that most invocations ofnewFileNotFoundErroracross the codebase use either a formatted message withStacktrace()or the corresponding overloaded method accepting an exception—both following the intended pattern.
- SetupFakeFileUseCase.kt: Uses
newFileNotFoundErrorwith a formatted error message and inlineStacktrace().- FakeFileSystemProvider.kt, SAFFileSystemProvider.kt, RegularFileSystemProvider.kt, WebDavClientV2.kt, AppSettingsInteractor.kt: Similar changes are evident, ensuring a consistent approach across the repository.
Overall, the changes approve the refactor, and no further modifications are necessary.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/optionDialog/OptionDialogViewModel.kt (1)
25-25:Details
❓ Verification inconclusive
LGTM! Method name change improves clarity.
The rename from
setCellElementstosetCellViewModelsbetter reflects the actual data being set and maintains consistency with other method names in the class.Let's verify that this method name change has been consistently applied across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining usages of the old method name and verify the new method exists in BaseScreenViewModel echo "Checking for any remaining usages of setCellElements:" rg "setCellElements" echo -e "\nVerifying the new method in BaseScreenViewModel:" rg -A 2 "setCellViewModels" "BaseScreenViewModel"Length of output: 358
LGTM! Method name change improves clarity, but please verify the base class update.
The renaming from
setCellElementstosetCellViewModelsaccurately reflects the data being set and aligns with the broader refactoring efforts. However, our initial check for the new method in theBaseScreenViewModelfile was inconclusive (the file wasn’t found using the provided path). Please ensure that:
- The base class file (likely named something like
BaseScreenViewModel.kt) exists in the repository and definessetCellViewModelsappropriately.- All derived view models consistently reference the new method name.
To further investigate, you can run the following shell script to locate the file and check for the new method definition:
#!/bin/bash echo "Searching for BaseScreenViewModel file:" fd -i "BaseScreenViewModel\.kt" echo -e "\nSearching for 'fun setCellViewModels' occurrences in Kotlin files:" rg "fun setCellViewModels" --type kotlinPlease verify the results of this script to confirm that the changes have been applied consistently.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/Preview.kt (3)
5-13: LGTM! Imports are correctly added for the new functionality.
63-74:Details
✅ Verification successful
Consider making the dummy text functions more reusable.
The implementation looks good, but could be more maintainable with a single parameterized function.
Consider refactoring to a single function with an enum parameter:
+enum class DummyTextLength { + SHORT, MEDIUM, LONG +} + +@Composable +fun getDummyText(length: DummyTextLength): String = + stringResource( + when (length) { + DummyTextLength.SHORT -> R.string.short_dummy_text + DummyTextLength.MEDIUM -> R.string.medium_dummy_text + DummyTextLength.LONG -> R.string.long_dummy_text + } + ) -@Composable -fun shortDummyText(): String = - stringResource(R.string.short_dummy_text) - -@Composable -fun mediumDummyText(): String = - stringResource(R.string.medium_dummy_text) - -@Composable -fun veryLongDummyText(): String = - stringResource(R.string.long_dummy_text)Let's verify the string resources exist:
🏁 Script executed:
#!/bin/bash # Search for dummy text string resources rg "short_dummy_text|medium_dummy_text|long_dummy_text" --type xmlLength of output: 865
Dummy Text Resource Functions Verified – Consider Single Parameter Refactor
The current implementation is correct and the string resources have been confirmed to exist. For improved maintainability, consider consolidating the three dedicated dummy text functions into one parameterized composable function. For example:
Files Reviewed:
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/Preview.kt(dummy text functions)app/src/main/res/values/untranslatable-strings.xml(string resource definitions)Refactoring Suggestion: Replace:
@Composable fun shortDummyText(): String = stringResource(R.string.short_dummy_text) @Composable fun mediumDummyText(): String = stringResource(R.string.medium_dummy_text) @Composable fun veryLongDummyText(): String = stringResource(R.string.long_dummy_text)with a single parameterized function:
+enum class DummyTextLength { + SHORT, MEDIUM, LONG +} + +@Composable +fun getDummyText(length: DummyTextLength): String = + stringResource( + when (length) { + DummyTextLength.SHORT -> R.string.short_dummy_text + DummyTextLength.MEDIUM -> R.string.medium_dummy_text + DummyTextLength.LONG -> R.string.long_dummy_text + } + )This refactoring would centralize dummy text handling, making future changes simpler and reducing code duplication.
58-61:Details
✅ Verification successful
Verify the ElementMargin constant.
The implementation looks good and follows Compose best practices. However, please ensure that the
ElementMarginconstant is properly defined and accessible.
🏁 Script executed:
#!/bin/bash # Search for ElementMargin constant definition rg "ElementMargin" --type kotlinLength of output: 940
ElementMargin Verified and Implementation Approved
The
ElementMarginconstant is defined inapp/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/compose/Dimens.ktas16.dpand is correctly used in theElementSpacecomposable as well as in other parts of the UI. The implementation adheres to Compose best practices.app/src/main/java/com/ivanovsky/passnotes/data/repository/file/remote/RemoteFileSystemProvider.java (2)
35-35: LGTM!The import of
Stacktraceis correctly placed and aligns with the error handling refactoring.
129-135: LGTM! Enhanced error handling with stack traces.The changes consistently include the original exception when creating error objects, which improves debugging capabilities by preserving the exception context.
app/src/main/res/layout/new_database_fragment.xml (6)
23-23: Addstateattribute for unified state binding in ErrorPanelView.
The newstate="@{viewModel.screenState}"attribute ensures that the view directly binds to the current UI state, aligning with the refactored error display strategy. Please verify that the corresponding view logic properly interprets this field.
26-26: Set background for ErrorPanelView to reflect error state.
Addingandroid:background="?kpErrorBackgroundColor"enhances the visual feedback when an error occurs. Confirm that thekpErrorBackgroundColorresource exists and aligns with your design guidelines.
29-29: Transition toscreenVisibilityHandlerin ErrorPanelView.
Replacing any legacy handler withapp:screenVisibilityHandler="@{viewModel.screenStateHandler}"is consistent with the new unified UI state handling. Ensure thatviewModel.screenStateHandleris implemented correctly to manage element visibility.
32-32: Integratestateattribute in ScreenStateView.
The inclusion ofstate="@{viewModel.screenState}"in ScreenStateView mirrors the update made in ErrorPanelView, promoting a consistent approach to binding UI state across components.
36-36: ApplyscreenVisibilityHandlerto ScreenStateView.
The addition ofapp:screenVisibilityHandler="@{viewModel.screenStateHandler}"ensures that the visibility handling in ScreenStateView follows the project’s updated paradigm. Please verify its functionality in context.
44-44: Update ScrollView to use the unified visibility handler.
The change from using the old state handler toapp:screenVisibilityHandler="@{viewModel.screenStateHandler}"in the ScrollView promotes consistent state and visibility management throughout the layout. It’s advisable to confirm that this update integrates seamlessly with other UI constraints.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/settings/database/changePassword/ChangePasswordDialogViewModel.kt (4)
8-8: Migration toBaseScreenViewModelis logical.
This transition aligns with your consolidated ViewModel architecture, ensuring consistent screen state management.
17-19: Defaulting toScreenState.data()on initialization.
Great approach for starting the screen in a ready-to-display state. Ensure you handle fallback states (e.g., empty, loading, error) as needed.
21-21: Visibility handler promotes clear UI state separation.
Defining a dedicated handler for visibility logic helps keep the ViewModel clean. Just make sure theChangePasswordScreenVisibilityHandleris well-documented.
39-39: Setting loading state before coroutine launch.
This correctly indicates to the UI that a background operation is in progress. Ensure you transition back to a non-loading state if any early returns happen.Could you confirm that all Coroutine cancellations or exceptions are caught and result in reverting to a non-loading state?
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/enterDbCredentials/EnterDbCredentialsViewModel.kt (5)
17-18: Imports properly reflect new base classes
These imports concisely introduce the newBaseScreenViewModelandDefaultScreenVisibilityHandler. No issues found.
31-33: Successful transition to the new BaseScreenViewModel
AdoptingBaseScreenViewModelwithinitialState = ScreenState.data()is a clean approach for standardized screen state handling. Good job removing directViewModelinheritance.
35-35: Check actual usage ofscreenVisibilityHandler
While it's fine to defineval screenVisibilityHandler = DefaultScreenVisibilityHandler(), the code does not appear to invoke any methods on it. Consider removing or referencing it for clarity, to avoid dead code.
66-66: Set error panel state
ReplacingErrorInteractorwithsetErrorPanelStateis an improvement for centralizing error display. This is a neat usage that aligns with your refactored error handling strategy.
118-118: New error handling approach
Similarly, usingsetErrorPanelState(getFileResult.error)unifies error handling. Ensuring that any stack trace is provided to the log or captured upstream would further benefit debugging.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/note/NoteViewModel.kt (2)
33-33: LGTM: Screen visibility handler update.The change from
DefaultScreenStateHandlertoDefaultScreenVisibilityHandleraligns with the broader refactor to improve UI state management.
638-642: LGTM: Screen state management improvement.The override of
setScreenStateproperly calls the superclass implementation and updates the FAB button visibility and menu items based on the current state.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/ViewModelFactory.kt (1)
9-11: LGTM! Clean implementation of ViewModelFactory.The class follows the Factory pattern correctly and uses Kotlin's vararg feature effectively for flexible dependency injection.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/widget/ScreenStateView.kt (6)
9-9: No issues with the new import.
This import is valid and necessary for using the newScreenStateclass.
19-19: Proceed with caution when callingapplyState(value)from the setter.
InvokingapplyStateon every assignment can be convenient for reactive UI changes but also means any late state assignment triggers immediate UI updates. Ensure this matches the intended lifecycle.
22-26: Binding initialization looks correct.
Instantiating the binding in the custom view constructor is a valid approach and simplifies layout inflation. Good job.
28-29: Guard clause is a clean way to handle null states.
The early return is straightforward and avoids unnecessary calls.
31-32: ExposingscreenStatedirectly toerrorViewcan facilitate consistent error UI.
This direct assignment is a good approach iferrorViewis designed to interpretScreenState.
34-36: Visibility toggles are straightforward and readable.
Mapping state flags to UI components ensures clarity. This is an effective way to maintain the UI’s responsiveness.app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/fake/FakeFileSystemProvider.kt (2)
18-18: LGTM!The import for
Stacktraceis correctly placed and necessary for the enhanced error handling implementation.
172-184:Details
✅ Verification successful
Overall implementation looks good.
The changes successfully implement the new error handling pattern. While using empty stack traces in a fake implementation is acceptable, capturing actual stack traces would provide more realistic behavior for testing scenarios where stack trace information is relevant.
Let's verify if
Stacktrace.capture()is available:
🏁 Script executed:
#!/bin/bash # Search for Stacktrace.capture() method definition ast-grep --pattern 'class Stacktrace { $$$ companion object { $$$ fun capture() { $$$ } $$$ } $$$ }'Length of output: 132
Overall implementation looks good.
The changes successfully implement the new error handling pattern and follow the project’s conventions. Since this fake file system provider is used for testing, using a basic
Stacktrace()is acceptable. However, capturing an actual stack trace—if possible via a method likeStacktrace.capture()—would simulate more realistic behavior in testing. Please verify manually using the above script whetherStacktrace.capture()is available in the codebase. If it exists, consider updating the fake implementation to use it for improved stack trace fidelity.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for occurrences of 'class Stacktrace' in Kotlin files:" rg "class Stacktrace" -t kotlin echo "Searching for occurrences of 'fun capture(' in Kotlin files:" rg "fun capture\s*\(" -t kotlinLength of output: 584
Overall implementation looks good.
After verifying the repository, we found that the
Stacktraceclass (located atapp/src/main/kotlin/com/ivanovsky/passnotes/domain/entity/exception/Stacktrace.kt) does not define acapture()method. Since this fake file system provider is used for testing, using the basicStacktrace()constructor is acceptable for now. However, if more realistic error handling is desired, you may consider implementing acapture()method within theStacktraceclass in the future.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groups/GroupsViewModel.kt (9)
36-36: No issues with importingStacktrace.The new import aligns with the refactored error handling approach, providing a way to capture stack traces for deeper insights.
58-59: Confirmed transition toBaseScreenViewModelandDefaultScreenVisibilityHandler.These imports indicate a move toward a standardized base view model and visibility handling strategy, which seems consistent with the broader refactoring and error-handling overhaul across the codebase.
132-132: Initialization ofscreenStateHandleris aligned with the new visibility handling approach.This instance likely integrates seamlessly with the new
BaseScreenViewModel. The usage appears consistent with the refactoring goals.
158-158:cellsLiveData instantiation is appropriate.Using
MutableLiveData<CellsData>is sensible for managing dynamic cell-based UI content. This approach is aligned with typical Android MVVM patterns.
313-313: AssigningCellsDatatocells.valueis correct.This correctly updates the UI layer with fresh cell view models. No concerns here.
540-540: Use ofsetErrorState(isAdded.error)appears consistent.This call matches the new error-handling pattern, simplifying the transition from legacy interactor-based error handling.
1129-1129: Creating anewGenericErrorwith aStacktrace.This addition ensures more context is captured for troubleshooting. No immediate concerns.
1162-1162: Consistent usage ofnewGenericErrorwithStacktrace.This replicates the pattern used elsewhere, strengthening error-tracking consistency.
1192-1193: OverridingsetScreenStateto extend base functionality.Calling
super.setScreenState(state)is correct, ensuring the base logic executes. The additional logic (updating FAB visibility, navigation panel visibility, etc.) is a natural extension of the new base view model approach.app/src/main/res/layout/unlock_fragment.xml (4)
25-25: Add state attribute for ErrorPanelView.
The addition ofstate="@{viewModel.screenState}"provides a clear binding to the view model's state, ensuring the error panel can dynamically adjust its display.
28-28: Set themed background for ErrorPanelView.
Assigningandroid:background="?kpErrorBackgroundColor"helps maintain a consistent visual theme. Please verify that?kpErrorBackgroundColoris defined in your theme resources.
30-30: Update onButtonClicked callback for ErrorPanelView.
Changing the callback fromonErrorPanelButtonClicked()toonErrorPanelActionButtonClicked()clarifies the intended action. Double-check that the view model now implementsonErrorPanelActionButtonClicked()accordingly.
35-35: Add state attribute for ScreenStateView.
Bindingstate="@{viewModel.screenState}"in the ScreenStateView improves the dynamic connection between the UI and the underlying state model.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/newdb/NewDatabaseViewModel.kt (8)
9-9: No issues with the newOperationErrorimport.
This aligns with the updated error handling approach introduced in this PR.
13-13: Import ofStacktraceis cohesive with the new error mechanism.
Ensures more thorough error messages and debugging data.
18-19: Imports forBaseScreenViewModelandDefaultScreenVisibilityHandlerseem correct.
These imports reflect the new architecture for unified screen state and visibility handling.
37-39: AdoptingBaseScreenViewModeland initializing state withScreenState.data()is a clean approach.
This change neatly centralizes state management in the base class.
41-41:DefaultScreenVisibilityHandlerusage looks good.
Make sure all references to screen state or visibility are routed through this component for consistency.
75-75: UsingsetScreenState(ScreenState.loading())aligns with new state handling.
No issues found here.
102-102: Passingresult.errordirectly tosetErrorPanelStateis consistent with the new design.
Make sure the upstreamresult.errorobjects always contain stack trace info if needed.
238-241: OverridingsetScreenStateto managedoneButtonVisibilityis a neat extension point.
Hiding the button on non-data states appears intentional and beneficial for user guidance.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginScreen.kt (13)
3-3: New import for background usage
The newly added import is used for.background()calls, specifically at line 142, to apply an error background color. The usage is valid, and there are no issues.
19-19: AndroidView import
Bringing inAndroidViewis appropriate for embedding custom views in Compose. Usage looks correct.
22-23: ScreenState and ScreenStateType imports
These imports integrate well with the new state-based approach for the screen, clarifying how different UI states are handled.
26-26: AppTheme import
Imported for color resources (e.g.,errorBackground). This is consistent with theming usage in the rest of the file.
35-35: ErrorPanelView import
This import introduces a custom view for displaying errors. It matches the usage in theAndroidViewblocks below.
46-52: Direct ViewModel references
Passing direct references (viewModel::onUrlChanged, etc.) streamlines event dispatch and avoids creating additional callback lambdas. This is a clean refactor and aligns with Compose best practices.
81-81: Usingwhen (state.screenState.type)
Switching onstate.screenState.typeclarifies state handling. This structure is more extensible than checking custom flags in the state object.
82-82: Handling NOT_INITIALIZED, LOADING states
Separating these states with aProgressIndicator()is straightforward and consistent. Ensure these states remain consistent with other loaders or placeholders in the codebase.
86-98: ERROR state with AndroidView
IncorporatingErrorPanelViewviaAndroidViewis appropriate to leverage existing views within Compose. The approach to setview.state = state.screenStateis correct. Be mindful of ensuring thestate.screenStatealways provides the error details needed byErrorPanelView.
118-118: Updated DataContent signature
ChangingDataContentto accept a genericServerLoginStaterather than a specialized data variant makes the composable more flexible.
120-120:state: ServerLoginStateparameter
This parameter switch aligns with the new unified approach to handle multiple screen states. Ensure thatDataContentgracefully handles any unrecognizedscreenStatetypes.
423-424: Preview withScreenState.data()for Git
Returning a data state for preview is appropriate to render the actual UI elements. This is a sound approach for design previews.
444-445: Preview withScreenState.data()for WebDav
Similar to the Git preview, this ensures the UI preview accurately reflects a data-laden state.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/unlock/UnlockViewModel.kt (12)
19-19: Good usage ofnewErrorMessagefor consistent error creation
This new import introduces a clear and consistent way to construct error messages throughout the codebase.
31-31: Ensure no sensitive data is leaked inStacktrace
Using a stack trace can be valuable for debugging, but verify that no sensitive or user-specific data is included before displaying it.
33-33: Clean extension usage
TheformatReadableMessageextension function is a nice approach for delivering user-friendly messages while maintaining code clarity.
48-49: Refactored toBaseScreenViewModelandDefaultScreenVisibilityHandler
Adopting a base screen view model and a default visibility handler aligns with a cohesive, reusable approach to screen state management.
76-77: Initialize with a loading state
Starting withScreenState.loading()reflects a best practice for asynchronous operations upon VM instantiation.
178-178: Renamed toonErrorPanelActionButtonClicked
This name clarifies that it’s the action button on the error panel. Ensure any old references match the new signature.
618-618: Consistent error formatting
UsingformatReadableMessageconsistently across the codebase improves readability and user experience.
739-741: File-not-found error withStacktrace()
Check if an empty or truncated stack trace is sufficient for user-facing scenarios to avoid unnecessary information disclosure.
752-754: Conflict error message withStacktrace()
Similarly, confirm whether the included stack trace might expose details that are not meant for end-users.
765-767: Generic sync error withStacktrace()
If you plan to show real trace data, consider sanitizing it or providing only relevant portions to users.
778-780: Auth error message includesStacktrace()
Ensure no sensitive credentials or personal user information are exposed within the trace.
875-876: OverridingsetScreenStatefor FAB visibility
Tying FAB visibility to the screen state is a concise, maintainable pattern.app/src/main/java/com/ivanovsky/passnotes/data/entity/OperationError.java (5)
3-5: Good use of nullability annotations.Using
@NonNulland@Nullablehelps with clearer contracts around the fields and parameters, reducing the risk of null-related bugs.
77-81: Read-only error fields are properly declared.Declaring these fields as
finalwith@NonNullor@Nullableclarifies their immutability and usage. This is a sound approach for avoiding accidental changes or null reference errors.
207-213: New error types are consistently integrated.Adding
CACHE_ERROR,ERROR_MESSAGE, andBIOMETRIC_DATA_INVALIDATED_ERRORhelps categorize errors more precisely. The inline comments regarding their usage provide clarity for future maintainers.
216-217: Private constructor enforces factory usage.Restricting constructor access ensures that the designated static methods are used for creating
OperationErrorinstances, promoting consistency and preventing unforeseen direct instantiations with missing fields.
223-236: Getter methods align with read-only design.Providing minimal getters while disallowing mutation is a solid approach for an error representation object, ensuring the error data remains stable after creation.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/service/DatabaseLockBroadcastReceiver.kt (3)
7-9: LGTM!The new imports align with the PR objectives of refactoring error handling.
19-19: LGTM!The initialization of
ResourceProviderfollows the established dependency injection pattern.
27-27: LGTM!The change to use
formatReadableMessageextension function is more idiomatic Kotlin and aligns with the PR objectives.app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/GetDatabaseUseCase.kt (2)
9-9: LGTM!The new import aligns with the PR objectives of enhancing error handling with stack traces.
28-30: LGTM!The addition of stack trace information enhances error reporting, and the formatting improves readability.
app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/EncodePasswordWithBiometricUseCase.kt (2)
10-10: LGTM!The new import aligns with the PR objectives of enhancing error handling with stack traces.
22-25: LGTM!The addition of stack trace information enhances error reporting, and the formatting improves readability.
app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/DecodePasswordWithBiometricUseCase.kt (2)
6-9: LGTM!The new imports align with the PR objectives of enhancing error handling with stack traces and using constants for error messages.
24-27: LGTM!The addition of stack trace information enhances error reporting, and the formatting improves readability.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/BaseScreenViewModel.kt (1)
27-38: LGTM! Well-structured error state handling.The new error handling methods provide a clean and consistent way to manage screen states:
setScreenStateis properly marked with@CallSupersetErrorStateandsetErrorPanelStateprovide convenient wrappers for common error scenariosapp/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/settings/app/AppSettingsInteractor.kt (1)
37-38: LGTM! Consistent error handling implementation.The error handling has been properly enhanced with stack traces in both methods:
getLogFileusesnewFileNotFoundErrorwith StacktraceremoveAllLogFilesusesnewGenericIOErrorwith Stacktrace and a specific error messageAlso applies to: 48-51
app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/GetUsedFileUseCase.kt (1)
27-37: LGTM! Well-structured error messages with stack traces.The error handling implementation is thorough and consistent:
- Uses formatted messages that include entity name and identifier
- Properly incorporates stack traces for debugging
- Maintains consistent error handling between both overloads
Also applies to: 48-58
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/ScreenState.kt (4)
3-6: Good use of @immutable for Compose optimization!The
@Immutableannotation helps Compose's runtime optimize recompositions by skipping unnecessary updates when the state hasn't changed.
10-10: Improved error handling with OperationErrorThe change from
errorText: String?toerror: OperationError?provides more structured error handling with additional context like stack traces.
14-30: Clean code: Improved readability with expression syntaxGood use of expression syntax and parentheses for boolean properties. The code is now more concise and easier to read.
49-69: Consistent error handling in factory methodsThe error factory methods now consistently use
OperationErrorinstead of raw strings, aligning with the new error handling strategy.app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/saf/SAFHelper.kt (1)
59-66: Enhanced error context with StacktraceGood addition of
Stacktrace()to provide more debugging context when file access fails.app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/webdav/WebDavNetworkLayer.kt (1)
36-40: Improved error handling with specific error typesGood improvements in error handling:
- Using specific error types for different HTTP status codes
- Including the original exception for better context
- Adding stack traces for enhanced debugging
app/src/main/kotlin/com/ivanovsky/passnotes/domain/usecases/RemoveUsedFileUseCase.kt (1)
27-34: Enhanced error tracing for database operationsGood addition of
Stacktrace()to provide better debugging context for database errors.app/src/main/kotlin/com/ivanovsky/passnotes/data/crypto/biometric/BiometricCipherProvider.kt (1)
6-6: LGTM! Error handling improvements look good.The changes consistently use the new error factory method with improved error reporting capabilities.
Also applies to: 28-28, 45-45
app/src/main/kotlin/com/ivanovsky/passnotes/domain/interactor/newdb/NewDatabaseInteractor.kt (1)
6-6: LGTM! Error handling improvements look good.The changes consistently use the new error factory methods with stack trace information for better error reporting.
Also applies to: 41-41, 54-57, 96-99
app/src/main/kotlin/com/ivanovsky/passnotes/domain/test/TestDataParser.kt (1)
26-31: LGTM! Error handling improvements look good.The changes consistently use the new error factory methods with stack trace information for better error reporting.
Also applies to: 48-48
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/TemplateDaoImpl.kt (1)
29-29: Address TODO comments in the code.There are several TODO comments that should be addressed:
- Line 29: Unsubscribe after db is closed
- Line 125-126: Template group in Recycle Bin handling
- Line 153: Refactoring note
Would you like me to help create issues to track these TODOs?
Also applies to: 125-126, 153-153
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/fake/FakeFileSystemSyncProcessor.kt (14)
27-27: LGTM!The import is correctly added to support the enhanced error handling with stack traces.
60-65: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
130-135: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
151-156: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
177-179: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
199-201: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
60-65: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
130-135: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
151-156: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
177-179: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
60-65: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
130-135: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
151-156: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
199-201: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/KeepassDatabaseRepository.kt (4)
18-18: LGTM!The import is correctly added to support the enhanced error handling with stack traces.
123-128: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
123-128: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
123-128: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/saf/SAFFileSystemProvider.kt (18)
17-17: LGTM!The import is correctly added to support the enhanced error handling with stack traces.
41-42: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
Also applies to: 46-46
194-196: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
204-206: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
214-216: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
224-226: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
41-42: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
46-46: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
194-196: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
204-206: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
214-216: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
224-226: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
41-41: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
46-46: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
194-196: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
204-206: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
214-216: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
224-226: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/regular/RegularFileSystemProvider.kt (22)
26-26: LGTM!The import is correctly added to support the enhanced error handling with stack traces.
59-64: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
Also applies to: 74-74
99-99: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
Also applies to: 104-109
128-128: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
Also applies to: 134-134
170-173: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
215-215: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
229-234: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
Also applies to: 240-245
253-258: LGTM!The error handling is correctly enhanced with stack trace information for better debugging.
59-64: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.Also applies to: 74-74
99-99: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.Also applies to: 104-109
128-128: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.Also applies to: 134-134
170-173: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
215-215: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
229-234: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
240-245: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
253-258: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
59-64: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.Also applies to: 74-74
99-99: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.Also applies to: 104-109
128-128: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.Also applies to: 134-134
170-173: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
215-215: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.
229-234: LGTM! Enhanced error reporting with stack trace.The addition of
Stacktrace()improves error diagnostics by providing stack trace information.Also applies to: 240-245, 253-258
app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/keepass/kotpass/KotpassNoteDao.kt (1)
82-90: LGTM! Error handling improvements look good.The addition of
Stacktrace()tonewDbErrorcalls enhances debugging capabilities by providing stack trace information. The changes are consistent across all error handling methods.Also applies to: 155-158, 164-170, 199-204
app/src/main/res/layout/storage_list_fragment.xml (3)
20-28: ErrorPanelView Attribute Update
The ErrorPanelView now binds itsstateto@{viewModel.screenState}and sets a background color using?kpErrorBackgroundColor. The attributeapp:screenVisibilityHandlerreplaces the old handler. However, note that the binding value is still@{viewModel.screenStateHandler}. Please verify that this is intentional and that your view model’s property name is correct for the new visibility handling convention.
30-35: ScreenStateView Attribute Update
The ScreenStateView now includes astateattribute and usesapp:screenVisibilityHandlerfor its visibility control. The implementation is clear and consistent.
37-47: DecoratedRecyclerView Update
The RecyclerView now also uses the newapp:screenVisibilityHandlerattribute instead of the old state handler attribute. As with the ErrorPanelView, the binding value remains@{viewModel.screenStateHandler}. Double-check if the view model should expose a property namedscreenVisibilityHandlerfor consistency across the refactored UI components.app/src/main/res/layout/enter_db_credentials_fragment.xml (3)
17-25: ErrorPanelView Update in Enter DB Credentials Layout
The ErrorPanelView has been updated to include astateattribute (bound to@{viewModel.screenState}) and anandroid:backgroundattribute set to?kpErrorBackgroundColor. The attribute replacement toapp:screenVisibilityHandlernow uses@{viewModel.screenVisibilityHandler}, which appears correct and consistent with the new design.
27-32: ScreenStateView Update
The ScreenStateView now correctly binds itsstateattribute and utilizesapp:screenVisibilityHandlerwith the expected view model property.
34-52: UnlockView Visibility Update
The UnlockView’s visibility-related attribute has been updated from using the old state handler toapp:screenVisibilityHandler(bound to@{viewModel.screenVisibilityHandler}). This is in line with the overall refactor. Ensure any downstream logic that reacts to visibility changes is updated accordingly.app/src/main/res/layout/file_picker_fragment.xml (4)
20-28: ErrorPanelView Update in FilePicker Layout
The ErrorPanelView now includes thestatebinding and sets a background color. It also replacesapp:screenStateHandlerwithapp:screenVisibilityHandler. Currently, the binding value is still@{viewModel.screenStateHandler}; please confirm that this property name is correct for your view model.
30-36: ScreenStateView Update
The ScreenStateView correctly integrates the newstateattribute andapp:screenVisibilityHandler, maintaining a consistent UI state binding.
38-49: TextView Enhancement
The TextView now includes the newapp:screenVisibilityHandlerattribute (bound to@{viewModel.screenStateHandler}). Verify that this binding aligns with your view model’s intended property naming, as some layouts usescreenVisibilityHandlerwhile others still referencescreenStateHandler.
51-61: DecoratedRecyclerView Update
The DecoratedRecyclerView has been updated to use the new visibility handler attribute. Its implementation is consistent with the rest of the layout; just confirm that the view model property (screenStateHandler) is correctly referenced or renamed as necessary.app/src/main/res/layout/note_editor_fragment.xml (4)
20-28: ErrorPanelView Update in NoteEditor Layout
The ErrorPanelView now bindsstateto@{viewModel.screenState}and addsandroid:background="?kpErrorBackgroundColor". It usesapp:screenVisibilityHandlerwith a binding value of@{viewModel.screenStateHandler}. Please verify that the view model property name aligns with the new naming convention you've established.
30-35: ScreenStateView Update
The ScreenStateView implementation is correct, binding thestateattribute and utilizing the new visibility handler attribute.
37-45: RecyclerView Update
The RecyclerView now uses theapp:screenVisibilityHandlerattribute to manage visibility in conjunction with its state binding. The change is straightforward and aligns with the refactor objectives.
47-60: FloatingActionButton Update
The FloatingActionButton has been updated to includeapp:screenVisibilityHandler, ensuring its visibility is managed consistently with other UI elements. The update appears correct and coherent with the layout changes.app/src/main/res/layout/dialog_resolve_conflict.xml (5)
41-43: TextView Update in Dialog
The TextView now includesapp:screenVisibilityHandlerbound to@{viewModel.screenVisibilityHandler}alongside its original state binding. This update is consistent with the new error display refactoring.
56-57: Remote Button Visibility Update
The Remote Button now sets itsapp:screenVisibilityHandlerattribute using@{viewModel.screenVisibilityHandler}, which complements its state binding.
70-71: Local Button Visibility Update
The Local Button also now includes the updatedapp:screenVisibilityHandlerattribute with a proper binding to@{viewModel.screenVisibilityHandler}. The update is implemented consistently.
84-85: Cancel Button Visibility Update
The Cancel Button now uses the newapp:screenVisibilityHandlerattribute, ensuring consistent visibility management. The binding to@{viewModel.screenVisibilityHandler}is clear and in agreement with the refactor.
88-96: ScreenStateView Update in Dialog
The ScreenStateView is now updated with astateattribute (@{viewModel.screenState}) and uses the newapp:screenVisibilityHandlerattribute bound to@{viewModel.screenVisibilityHandler}. This provides a unified mechanism for reflecting UI state changes across the dialog.app/src/main/res/layout/debug_menu_fragment.xml (1)
43-51: ConstraintLayout’s Visibility Handler Usage
Within the nested ConstraintLayout (lines 43-51), the binding forapp:screenVisibilityHandleris updated to@{viewModel.screenVisibilityHandler}. This is consistent with the intended refactor in this layout. No additional concerns here except to recheck overall consistency across all layouts.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialog.kt (3)
28-31: Confirm the composable screen’s test coverage.
ReportErrorDialogScreen(viewModel)is central to the dialog’s display logic. Ensure you have test cases validating that relevant UI states, errors, and user interactions are properly handled.
33-43: Ensure event observers are cleaned up upon dialog dismissal.LiveData is lifecycle-aware, which typically prevents leaks. However, double-check there’s no scenario in which the observer lingers beyond the dialog lifecycle or re-triggers unintentionally.
45-56: LGTM.Providing a static
newInstancemethod and a uniqueTAGfor logging is a neat convention.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialogViewModel.kt (2)
14-26: LGTM! Well-structured ViewModel with clean dependency injection.The ViewModel follows MVVM pattern with proper state management using MutableStateFlow and SingleLiveEvent for navigation.
60-73: LGTM! Clean state creation logic.The state creation method is well-organized and handles null stacktrace gracefully.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/dialog/reportErrorDialog/ReportErrorDialogScreen.kt (2)
44-54: LGTM! Clean separation of concerns.The main composable function properly collects state and delegates to the implementation.
144-168: LGTM! Comprehensive preview functions.The preview functions cover both collapsed and expanded states, which is great for UI development.
app/build.gradle (1)
11-11: Version increment looks good.The minor version increment from 12 to 13 aligns with the significant refactoring of error handling.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/core/widget/ErrorPanelView.kt (9)
6-7: Use ofFrameLayoutis appropriate.No concerns with these new imports. They are needed for the updated functionality and provide a suitable foundation for the changes elsewhere in the file.
9-10: No issues with these imports.All are relevant to the new error handling mechanism, view binding, and utility functions.
Also applies to: 12-13, 14-15, 17-17
19-19: Transition toFrameLayoutis aligned with a simpler layout approach.Shifting from a more complex ConstraintLayout to FrameLayout is often beneficial for a custom error panel.
24-24: Straightforward property declarations.Defining a nullable
ScreenStateand callingapplyStatein its setter is acceptable for this use case.Also applies to: 27-27, 30-30
33-34: Good use of view binding.Storing the current error as a separate nullable field helps keep the UI logic clean.
37-41: Initialization block is clear and effective.Inflating the binding in
initis a typical and concise approach for custom views.
43-46: Listeners are properly assigned.Using null-safe calls (
?.) prevents crashes when listeners are unset. Implementation looks clean.
48-49:applyStateimplementation is straightforward.Properly updates the UI elements based on the current error state; logic is clear and maintainable.
Also applies to: 51-51, 53-53, 54-54, 55-55, 57-57
60-61: Simple close action.Toggling visibility to hide the panel is appropriate. No issues here.
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groupEditor/GroupEditorViewModel.kt (2)
17-18: New imports align with the updated architecture.These imports reflect the shift towards a unified base view model and seem correct and consistent with the broader refactor goals.
67-67:Details
❓ Verification inconclusive
Ensure stack trace is captured if needed.
The new error handling pattern may require passing a
Stacktraceparameter for full debugging context. Verify thatloadDataResult.errorincludes the stack trace before callingsetErrorState.
🏁 Script executed:
#!/bin/bash # Searching for usage of stack trace in error creation to confirm whether it's passed. rg -A 4 'OperationError\('Length of output: 13676
Ensure full debugging context via proper stack trace propagation
At
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/groupEditor/GroupEditorViewModel.kt:67, the error state is set with:setErrorState(loadDataResult.error)Please verify that the error object (
loadDataResult.error) is constructed using one of theOperationErrorfactory methods that includes a stack trace (for example, methods likenewGenericError,newFileAccessError, etc., which accept aStacktraceparameter). If the error isn’t capturing the full stack trace, consider updating the error creation to include it for complete debugging context.app/src/main/kotlin/com/ivanovsky/passnotes/presentation/debugmenu/DebugMenuViewModel.kt (13)
12-12: Adoption ofnewErrorMessage,Stacktrace, and new base classes looks good.
This change makes the error handling more explicit and ties the view model to a standardized base and visibility handler.Also applies to: 21-21, 26-27
45-47: Switching fromViewModeltoBaseScreenViewModel.
This is a clean way to unify screen state management. Ensure that any specific lifecycle or injection logic from the old approach is accounted for in the new base class.
50-50: New screen visibility handler instantiation.
UsingDefaultScreenVisibilityHandlerstandardizes UI toggling. This is consistent with the refactor’s pattern of centralizing state handling.
106-106: Refactored error handling inonReadButtonClicked.
InvokingsetScreenState(ScreenState.data())before the operation and subsequently callingsetErrorPanelStateon failure aligns with the new approach.Also applies to: 118-118
135-135: Checking screen state updates inonWriteButtonClicked.
InvokingsetScreenStateand deferring errors tosetErrorPanelStateensures consistent UI feedback.Also applies to: 155-155
172-172: Screen state and error handling inonNewButtonClicked.
The current approach to reset state before invoking the operation and callingsetErrorPanelStateon failure is consistent.Also applies to: 188-188
200-200: Consistent pattern inonCheckExistsButtonClicked.
Setting a “data” state up front, then funneling errors tosetErrorPanelStateis a clear approach.Also applies to: 212-212
220-220: Refactored callbacks inonOpenDbButtonClicked.
Updating the screen state and showing errors via the new method is straightforward and consistent.Also applies to: 237-237
262-262: LeveragingsetScreenStateinonEditDbButtonClicked.
This minor adjustment aligns with the new base view model’s approach.
277-277: Updated screen state usage inonCloseDbButtonClicked.
The view model’s consistency in handling UI state and errors is maintained here as well.Also applies to: 288-288
302-302: Consistent refactor inonAddEntryButtonClicked.
Same concise pattern: set “data” state, perform operation, then usesetErrorPanelStateif needed.Also applies to: 312-312
376-376: Refined error handling for file retrieval methods.
onFilePickedandonGetRootButtonClickednow channel errors through the newsetErrorPanelState. Good consistency.Also applies to: 390-390
429-429: EnhancedshowExampleDiffwith loading and error states.
UsingsetScreenState(ScreenState.loading())for long-running tasks and reverting todataor error states is excellent for user feedback.Also applies to: 448-448, 450-450
app/src/main/kotlin/com/ivanovsky/passnotes/presentation/serverLogin/ServerLoginViewModel.kt (11)
19-19: No issues with the new import.
Bringing inScreenStateis appropriate and consistent with the refactored approach.
64-70: This state update is clear and consistent.
UsingsetScreenStateensures menu items are recalculated and keeps the logic uniform.
72-78: No concerns with password visibility update.
The function correctly usessetScreenState. Good work.
92-94: Straightforward navigation.
No issues here. User flow is clear.
142-181: Credential building logic looks correct.
Implementation clearly distinguishes between SSH, Git, and Basic credentials. Good job.
183-215: Ssh option selection flow looks good.
The 500 ms delay is a known workaround forExposedDropdownMenu. No further concerns.
221-231: Consistent usage ofsetScreenState.
This block properly updates the UI state and recalculates menu items.
233-239: Clearing errors is well-handled.
Resetting to a data state and nullifying errors is straightforward.
273-311: Initial state creation is clean.
No concerns with this straightforward approach to building default values.
334-339: Menu visibility check looks fine.
Hiding the menu in non-data states aligns with typical UX patterns.
96-140:Details
✅ Verification successful
Verify result emission after
router.exit().
Callingrouter.exit()beforerouter.sendResult(...)might skip sending the result to the previous screen if the navigation stack is already popped. Double-check the library’s behavior to ensure the screen result is delivered as intended.
🏁 Script executed:
#!/bin/bash # Searching usage patterns of router.exit() and router.sendResult() across the codebase rg -A 5 "router\.exit" rg -A 5 "router\.sendResult"Length of output: 26229
Verification complete: Router result emission confirmed
After reviewing the routing patterns across the codebase—in particular, the consistent usage in FilePickerViewModel, EnterDbCredentialsViewModel, StorageListViewModel, and SetupOneTimePasswordViewModel—it appears that calling
router.exit()immediately beforerouter.sendResult(...)is an intentional and proven pattern. The evidence suggests that the router implementation reliably delivers the screen result even after the navigation exit is triggered.No modifications are necessary in this section unless the router's internal behavior changes in the future.
Summary by CodeRabbit