commit 49bd0e4c2ece121d164a9d53cbf9c8290488ba3e parent fc157dc79e2b97ed4f5b9c6ea2c73dfdd2dfc174 Author: fmasalha <fmasalha@mozilla.com> Date: Wed, 10 Dec 2025 19:46:29 +0000 Bug 2002304 - Added safeguards around stale bookmarks multi select move state r=android-reviewers,matt-tighe Differential Revision: https://phabricator.services.mozilla.com/D274434 Diffstat:
6 files changed, 52 insertions(+), 13 deletions(-)
diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksAction.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksAction.kt @@ -149,4 +149,5 @@ internal sealed class DeletionDialogAction : BookmarksAction { internal sealed class SnackbarAction : BookmarksAction { data object Undo : SnackbarAction() data object Dismissed : SnackbarAction() + data object SelectFolderFailed : SnackbarAction() } diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksMiddleware.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksMiddleware.kt @@ -208,11 +208,15 @@ internal class BookmarksMiddleware( return@also } scope.launch { - preReductionState.createMovePairs()?.forEach { - val result = bookmarksStorage.updateNode(it.first, it.second) - if (result.isFailure) { - reportResultGlobally(BookmarksGlobalResultReport.SelectFolderFailed) - } + val successes = preReductionState.createMovePairs() + ?.mapNotNull { item -> + bookmarksStorage.updateNode(item.first, item.second) + .takeIf { result -> + result.isSuccess + } + } + if (successes.isNullOrEmpty()) { + context.store.dispatch(SnackbarAction.SelectFolderFailed) } context.store.tryDispatchLoadFor(preReductionState.currentFolder.guid) } @@ -377,6 +381,7 @@ internal class BookmarksMiddleware( } is InitEditLoaded, SnackbarAction.Undo, + SnackbarAction.SelectFolderFailed, is OpenTabsConfirmationDialogAction.Present, OpenTabsConfirmationDialogAction.CancelTapped, DeletionDialogAction.CancelTapped, @@ -691,7 +696,10 @@ private suspend fun BookmarksStorage.hasDesktopBookmarks(): Boolean { private fun BookmarksState.createMovePairs() = bookmarksMultiselectMoveState?.let { moveState -> moveState.guidsToMove.map { guid -> - val bookmarkItem = bookmarkItems.first { it.guid == guid } + val bookmarkItem = bookmarkItems.firstOrNull { it.guid == guid } + if (bookmarkItem == null) { + return null + } guid to BookmarkInfo( moveState.destination, // Setting position to 'null' is treated as a 'move to the end' by the storage API. diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksReducer.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksReducer.kt @@ -217,6 +217,10 @@ private fun BookmarksState.handleSnackbarAction(action: SnackbarAction): Bookmar ) } } + + SnackbarAction.SelectFolderFailed -> { + this.copy(bookmarksSnackbarState = BookmarksSnackbarState.SelectFolderFailed) + } } } diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksScreen.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksScreen.kt @@ -273,6 +273,9 @@ private fun BookmarksList( val (id, titleOrCount) = state.undoSnackbarText() stringResource(id, titleOrCount) } + BookmarksSnackbarState.SelectFolderFailed -> { + stringResource(R.string.bookmark_error_select_folder) + } is BookmarksSnackbarState.BookmarkMoved -> { stringResource( R.string.bookmark_moved_single_item, @@ -282,9 +285,6 @@ private fun BookmarksList( ), ) } - BookmarksSnackbarState.SelectFolderFailed -> { - stringResource(R.string.bookmark_error_select_folder) - } else -> "" } @@ -316,13 +316,13 @@ private fun BookmarksList( onDismissPerformed = { store.dispatch(SnackbarAction.Dismissed) }, ) } - is BookmarksSnackbarState.BookmarkMoved -> scope.launch { + BookmarksSnackbarState.SelectFolderFailed -> scope.launch { snackbarHostState.displaySnackbar( message = snackbarMessage, onDismissPerformed = { store.dispatch(SnackbarAction.Dismissed) }, ) } - BookmarksSnackbarState.SelectFolderFailed -> scope.launch { + is BookmarksSnackbarState.BookmarkMoved -> scope.launch { snackbarHostState.displaySnackbar( message = snackbarMessage, onDismissPerformed = { store.dispatch(SnackbarAction.Dismissed) }, diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksTelemetryMiddleware.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksTelemetryMiddleware.kt @@ -99,8 +99,9 @@ internal class BookmarksTelemetryMiddleware : Middleware<BookmarksState, Bookmar MetricsUtils.recordBookmarkMetrics(MetricsUtils.BookmarkAction.DELETE, source) } } - - SnackbarAction.Undo -> Unit + SnackbarAction.SelectFolderFailed, + SnackbarAction.Undo, + -> Unit } } diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/bookmarks/BookmarksMiddlewareTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/bookmarks/BookmarksMiddlewareTest.kt @@ -1101,6 +1101,31 @@ class BookmarksMiddlewareTest { } @Test + fun `GIVEN we are in the select folder screen and we are moving multiple bookmarks and bookmark items is empty WHEN the folder is selected and back button is clicked THEN we show a snackbar describing the error`() = runTestOnMain { + val tree = generateBookmarkTree() + val middleware = buildMiddleware() + val store = middleware.makeStore( + initialState = BookmarksState.default.copy( + bookmarkItems = listOf(), + bookmarksSelectFolderState = BookmarksSelectFolderState( + outerSelectionGuid = tree.guid, + innerSelectionGuid = tree.children!!.first { it.type == BookmarkNodeType.FOLDER }.guid, + ), + bookmarksMultiselectMoveState = MultiselectMoveState( + guidsToMove = listOf("item guid 1", "item guid 2"), + destination = "Some other location", + ), + ), + ) + + assertEquals(BookmarksSnackbarState.None, store.state.bookmarksSnackbarState) + + store.dispatch(BackClicked) + + assertEquals(BookmarksSnackbarState.SelectFolderFailed, store.state.bookmarksSnackbarState) + } + + @Test fun `GIVEN a user is on the edit screen with nothing on the backstack WHEN delete is clicked THEN pop the backstack, delete the bookmark and exit bookmarks`() = runTestOnMain { var exited = false exitBookmarks = { exited = true }