Bugfix FXIOS-15296 [Bookmarks Panel] Fix for erroneous warning popup when deleting an empty bookmarks folder (when contents deleted via search model) by ih-codes · Pull Request #32871 · mozilla-mobile/firefox-ios

ih-codes · 2026-04-01T22:18:04Z

📜 Tickets

💡 Description

The bug is a bit hard to describe, see the video in FXIOS-15296.

The tldr; is that when you are in search mode, and use the long-press context menu to delete all the bookmarks in a bookmark folder, then exit search mode, and then try to delete that folder, a warning pops up for deleting a "non empty" folder. We expect no warning should pop up, because the folder is now actually empty.

Fix

This PR fixes the folder alert issue by adding an extra reload of the bookmarks tree after a bookmark deletion while in search mode.

If a user is extremely fast I'm sure you'd still see the warning, but re-fetching bookmarks seemed quite fast for me (though I don't have that many bookmarks synced). I think there is a low chance of this edge case coming up so I'm not too concerned about this, and it's documented in the code.

Background Info

The fix is to reload the bookmarks tree after deleting a bookmark while in search mode. Unfortunately the backing bookmarks data and the search result bookmarks data are two separate backing arrays that need to both be updated. This is less than ideal. 😕

Ideally the search results would just filter the real backing data and we'd fetch the bookmarks tree with recursive: true, but writing logic to update the tree in place (for optimistic UI updates like deleting a bookmark) will take some work. There is a separate ticket for that here (FXIOS-15309 / #32870), but I don't expect anyone will tackle this anytime soon.

Testing Instructions

ℹ️ To test this new feature, please turn the bookmarks-search-feature to true in the nimbus feature yaml (no debug toggle yet, sorry).

📝 Checklist

I filled in the ticket numbers and a description of my work
I updated the PR name to follow our PR naming guidelines
I ensured unit tests pass and wrote tests for new code
If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
If adding telemetry, I read the data stewardship requirements and will request a data review
If adding or modifying strings, I read the guidelines and will request a string review from l10n
If needed, I updated documentation and added comments to complex code

ih-codes · 2026-04-01T22:25:19Z

Hmmmmm the diff is too big so something went wonky with my branching off of branches strategy. I'll let you know when this is actually... reviewable. 🤨

Edit: Fixed!

mergify · 2026-04-01T22:27:39Z

This pull request has conflicts when rebasing. Could you fix it @ih-codes? 🙏

ih-codes · 2026-04-01T22:32:08Z

                MainActor.assumeIsolated {
                    guard result.isSuccess else { return }
-                    self.removeBookmarkShortcut()
+                    Self.removeBookmarkShortcut(withBookmarksHandler: self.bookmarksHandler)


I discovered a retain issue in this method as I was implementing my new unit tests. As a fix, I made it a static method since it doesn't really need self.

ih-codes · 2026-04-01T22:32:52Z

    let bookmarkFolderGUID: GUID
    var bookmarkFolder: FxBookmarkNode?
    var isShowingSearchResults = false
+    private var currentSearchQuery: String?


Added this since I refresh the current search results via a fresh bookmarks tree query after removing a bookmark.

ih-codes · 2026-04-01T22:33:38Z


    @MainActor
-    func removeBookmarkShortcut(quickAction: QuickActions)
+    static func removeBookmarkShortcut(


Changes in this file were to increase testability and fix a retain cycle detected in unit tests.

ih-codes · 2026-04-01T22:34:36Z

-/// A mock handler that returns a configurable `BookmarkFolderData` for search tests.
-private final class SearchableMockBookmarksHandler: BookmarksHandler, @unchecked Sendable {


Turns out this was basically just a copy of our other Mock BookmarksHandler, so I deleted it.

mergify · 2026-04-01T23:11:15Z

This pull request has conflicts when rebasing. Could you fix it @ih-codes? 🙏

…lder is non-empty when we deleted that folder's contents via bookmarks search.

… backing arrays are updated. Consolidate 2 bookmarks handler mocks into one. Fix retain issue with the quick actions extension default implementation by making it a static method.

mobiletest-ci-bot · 2026-04-02T15:38:44Z

	Warnings
⚠️	Deferred class is used in file firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockBookmarksHandler.swift at line 110. Please replace with completion handler instead.

	Messages
📖	Project coverage: 41.01%

💪 Quality guardian

3 tests files modified. You're a champion of test coverage! 🚀

🥇 Perfect PR size

Smaller PRs are easier to review. Thanks for making life easy for reviewers! ✨

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

✅ New file code coverage

No new file detected so code coverage gate wasn't ran.

Client.app: Coverage: 39.4

File	Coverage
BrowserViewController.swift	35.03%	⚠️
TabManagerMiddleware.swift	36.69%	⚠️
BookmarksViewController.swift	14.81%	⚠️
BookmarksPanelViewModel.swift	74.12%	✅
CanRemoveQuickActionBookmark.swift	100.0%	✅

libStorage.a: Coverage: 55.46

File	Coverage
RustPlaces.swift	66.32%	✅

Generated by 🚫 Danger Swift against 16f58ed

Foxbolts

Looks good, thanks for fixing this.

I think you would need to have a pretty large number of bookmarks for the DB update to be slow enough to warrant putting a fix in for the race condition, so I agree with you that it's not much of a concern

github-actions · 2026-04-06T16:43:56Z

🚀 PR merged to main, targeting version: 150.0

ih-codes requested a review from Foxbolts April 1, 2026 22:18

ih-codes requested a review from a team as a code owner April 1, 2026 22:18

ih-codes force-pushed the ih/FXIOS-15240-bugfixes-after-vm-tweaks branch from e3901f8 to 0eb4fe6 Compare April 1, 2026 22:22

ih-codes force-pushed the ih/FXIOS-15296-fix-empty-folder-alert branch from f920284 to 163c970 Compare April 1, 2026 22:28

ih-codes commented Apr 1, 2026

View reviewed changes

Base automatically changed from ih/FXIOS-15240-bugfixes-after-vm-tweaks to main April 1, 2026 23:09

ih-codes added 3 commits April 2, 2026 09:25

Reload the data so the user does not get the popup that suggests a fo…

e7b090a

…lder is non-empty when we deleted that folder's contents via bookmarks search.

Add new unit test for removing bookmarks to ensure bookmarks panel VM…

ecec25a

… backing arrays are updated. Consolidate 2 bookmarks handler mocks into one. Fix retain issue with the quick actions extension default implementation by making it a static method.

Edit documentation and add tickets.

16f58ed

ih-codes force-pushed the ih/FXIOS-15296-fix-empty-folder-alert branch from 163c970 to 16f58ed Compare April 2, 2026 15:25

ih-codes mentioned this pull request Apr 2, 2026

Fix #32719: Use displayedBookmarkNodes instead of bookmarkNodes for U… #32721

Closed

Foxbolts approved these changes Apr 2, 2026

View reviewed changes

ih-codes merged commit fdb9947 into main Apr 6, 2026
9 checks passed

ih-codes deleted the ih/FXIOS-15296-fix-empty-folder-alert branch April 6, 2026 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix FXIOS-15296 [Bookmarks Panel] Fix for erroneous warning popup when deleting an empty bookmarks folder (when contents deleted via search model)#32871

Bugfix FXIOS-15296 [Bookmarks Panel] Fix for erroneous warning popup when deleting an empty bookmarks folder (when contents deleted via search model)#32871
ih-codes merged 3 commits intomainfrom
ih/FXIOS-15296-fix-empty-folder-alert

ih-codes commented Apr 1, 2026

Uh oh!

ih-codes commented Apr 1, 2026 •

edited

Loading

Uh oh!

mergify Bot commented Apr 1, 2026

Uh oh!

ih-codes Apr 1, 2026

Uh oh!

ih-codes Apr 1, 2026

Uh oh!

ih-codes Apr 1, 2026

Uh oh!

ih-codes Apr 1, 2026

Uh oh!

mergify Bot commented Apr 1, 2026

Uh oh!

mobiletest-ci-bot commented Apr 2, 2026

Uh oh!

Foxbolts left a comment

Uh oh!

Uh oh!

github-actions Bot commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		/// A mock handler that returns a configurable `BookmarkFolderData` for search tests.
		private final class SearchableMockBookmarksHandler: BookmarksHandler, @unchecked Sendable {

Conversation

ih-codes commented Apr 1, 2026

📜 Tickets

💡 Description

Fix

Background Info

Testing Instructions

📝 Checklist

Uh oh!

ih-codes commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify Bot commented Apr 1, 2026

Uh oh!

ih-codes Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

ih-codes Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

ih-codes Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

ih-codes Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Apr 1, 2026

Uh oh!

mobiletest-ci-bot commented Apr 2, 2026

💪 Quality guardian

🥇 Perfect PR size

💬 Description craftsman

✅ New file code coverage

Client.app: Coverage: 39.4

libStorage.a: Coverage: 55.46

Uh oh!

Foxbolts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ih-codes commented Apr 1, 2026 •

edited

Loading