commit 01f977e353a442ef5b29b4eef91a74ca93a353ae parent 281064b3d400db2eb0637de24306663c44727e1c Author: fmasalha <fmasalha@mozilla.com> Date: Wed, 17 Dec 2025 19:20:05 +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, 54 insertions(+), 17 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 @@ -153,4 +153,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) } @@ -393,6 +397,7 @@ internal class BookmarksMiddleware( SelectFolderAction.SearchClicked, SelectFolderAction.SearchDismissed, is InitEditLoaded, + SnackbarAction.SelectFolderFailed, SnackbarAction.Undo, is OpenTabsConfirmationDialogAction.Present, OpenTabsConfirmationDialogAction.CancelTapped, @@ -709,7 +714,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 @@ -250,6 +250,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 @@ -277,6 +277,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, @@ -286,9 +289,6 @@ private fun BookmarksList( ), ) } - BookmarksSnackbarState.SelectFolderFailed -> { - stringResource(R.string.bookmark_error_select_folder) - } else -> "" } @@ -320,13 +320,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 @@ -103,8 +103,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 @@ -1133,6 +1133,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 } @@ -1805,8 +1830,7 @@ class BookmarksMiddlewareTest { `when`(bookmarksStorage.countBookmarksInTrees(listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id))).thenReturn(0u) `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id)).thenReturn(Result.success(generateBookmarkTree())) `when`(bookmarksStorage.updateNode(any(), any())).thenReturn(Result.failure(IllegalStateException())) - var reported: BookmarksGlobalResultReport? = null - val middleware = buildMiddleware(reportResultGlobally = { reported = it }) + val middleware = buildMiddleware() val store = middleware.makeStore( initialState = BookmarksState.default.copy( @@ -1819,10 +1843,9 @@ class BookmarksMiddlewareTest { ), ), ) - store.dispatch(BackClicked) - assertEquals(BookmarksGlobalResultReport.SelectFolderFailed, reported) + assertEquals(BookmarksSnackbarState.SelectFolderFailed, store.state.bookmarksSnackbarState) } private fun buildMiddleware(