Refactor FXIOS-13499 [Trending Searches] recent searches to use A/S #29797

cyndichin · 2025-10-03T16:04:10Z

📜 Tickets

💡 Description

This PR handles adding recent search terms to our history storage as well as fetching the list.

Background
Instead of using the initial simple cache of recent searches that was implemented here, we update our DefaultRecentSearchProvider to use the history storage via Rust Places instead to store our search terms (similar to what's being done on Android).

Currently, Android is storing more than just search terms, but for the sake of this feature we only care about storing search terms.

Also, add a new ToolbarAction associated with submitting a search term so we can trigger addRecentSearch that stores the search term and associated url in history storage.
Rename test file to DefaultRecentSearchProvider

With this PR, we can now see the search terms being saved locally in places.db, which was previously empty.

Note: On Desktop, they are not using application services and each search engine has its own set of search terms, which was our the previous implementation. However, to maintain parity with Android we are using application services.

Testing

Currently, there is no UI for recent searches, but you can add a breakpoint in retrieveRecentSearches in SearchViewModel to confirm that you are receiving the proper search terms. This PR does not include updating the zero search logic to handle recent searches, that will be in a separate PR. I did notice the first time you write to the db, it seems to take a while to add searches, but after seems to work fine.

Make sure to turn on both FF for now, you can revert this commit: ffcca39

Here's my demo:

Screen.Recording.2025-10-03.at.12.18.38.PM.mov

📝 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

cyndichin · 2025-10-03T16:07:55Z

firefox-ios/Client/Frontend/Browser/SearchEngines/RecentSearchProvider.swift


-/// A provider that manages recent search terms for a specific search engine.
+/// A provider that manages recent search terms from a user's history storage.
 struct DefaultRecentSearchProvider: RecentSearchProvider {


If you are reviewing this file, I would say to review the Raw file since it might be easier as the whole file is basically rewritten.

If you are wondering why we are using Int64.min in this file below, this is because I am following Android's implementation that uses Long.MIN_VALUE and there's no method to fetch X number of search terms currently.

So, the min is used in terms of the date timestamp. Does this mean we're fetching everything from all of time? 🤔

Does this mean we're fetching everything from all of time?

No it's limited to a 1000. Anyways I didn't like how we have to get x items and have to filter it out to the limit we want. I opened mozilla/application-services#7002 to add a convenience method that returns n most recent items. This is not a blocker though. We can merge this and update later when the new method is available. @cyndichin can you please add a //TODO(FXIOS-xxx)

Thanks @issammani for opening that PR, it'll be helpful for Android too! You 🪨 ! It seems requirements may have change in that we may want to fetch all recent searches (with no limits). Still TBD, but the method looks great regardless!

Updated here: d62599d

cyndichin · 2025-10-03T16:08:51Z

firefox-ios/Client/Frontend/Browser/Toolbars/Redux/ToolbarAction.swift

    case didEnterSearchTerm
+    // User submitted a search term to load the search request
+    case didSubmitSearchTerm


@thatswinnie I wasn't sure why we need to make this distinction, but didEnterSearchTerm did not seem to be triggered as I thought it would be. Maybe we can add a comment to clarify the difference. We can chat live on this as well!

It seems like we are just using this action in the ToolbarMiddleware so we should make this a ToolbarMiddlewareAction instead.

thanks @thatswinnie ! I believe that the naming is based on where the action is called and not the consequences.

Here's our wiki guidelines on that:

When it comes to naming convention, we dispatch general action names from the view and the middleware dispatches middleware actions. i.e. `GeneralActionType.show` lives in the view and `GeneralMiddlewareActionType.show` lives in the middleware.

I did write these when I was discussing with @OrlaM on Redux and think I brought it to the team in the early days. Happy to sync or update this if something has changed and I'm unaware.

https://github.com/mozilla-mobile/firefox-ios/wiki/Redux-Guidelines---FAQs#faqs

mobiletest-ci-bot · 2025-10-03T16:16:06Z

	Messages
📖	Project coverage: 38.12%

💪 Quality guardian

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

🧩 Neat Piece

This PR changes 492 lines. It's a substantial update,
but still review-friendly if there’s a clear description. Thanks for keeping things moving! 🚀

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

🦊 BrowserViewController Check

We’re tracking the size of BrowserViewController.swift to keep it healthy.

✨ Change in file size: +4 lines

❌ Per-file test coverage gate

The following changed file(s) are below 35.0% coverage:

File	Coverage	Required
`firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift`	32.2%	35.0%

Client.app: Coverage: 37.39

File	Coverage
BrowserViewController.swift	32.2%	⚠️
ToolbarAction.swift	100.0%	✅
SearchViewModel.swift	65.88%	✅
RecentSearchProvider.swift	89.36%	✅
ToolbarMiddleware.swift	93.24%	✅

libStorage.a: Coverage: 56.54

File	Coverage
RustPlaces.swift	67.32%	✅

Generated by 🚫 Danger Swift against ab70f7f

cyndichin · 2025-10-03T16:34:11Z

Happy to chat live about this PR as well! Tagging @thatswinnie for toolbar, @issammani for A/S stuff and @ih-codes who has been reviewing the recent searches PRs.

ih-codes

A first pass with some thoughts, I didn't take the time to run it and will leave that to QA or other reviewers unless you'd like me to dig in more. 👌

ih-codes · 2025-10-06T17:49:48Z

...Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+URLBarDelegate.swift


        finishEditingAndSubmit(searchURL, visitType: VisitType.typed, forTab: tab)
+
+        guard isRecentSearchEnabled else { return }


nit; I would like this logic to be encased in an if so someone doesn't go appending extra work at the end of this function not realizing it's barred by a feature flag guard. It's just clearer to me to wrap the conditional code in an if for stuff like this.

I compromised and moved into its own method, let me know if any issues?

Updated commit - 3a931ea

Perfecto! 👌

ih-codes · 2025-10-06T17:52:25Z

firefox-ios/Client/Frontend/Browser/Search/SearchViewModel.swift

+            ensureMainThread { [weak self] in
+                self?.delegate?.reloadTableView()
+            }


Alternatively, you could adapt the loadRecentSearches function signature to use a @MainActor completion. Then in the loadRecentSearches method you call the completion on the main thread, and here you wouldn't need this.

I just suggest it since loading the engines somewhat suggests to me that we'd perform some UI work after some networking. 😄

I was trying to stay away from mixing completion handlers / modern swift concurrency here and we're more explicit in what we want to ensure is on main thread. However, it does seem cleaner in theory though and can't think of a case where we wouldn't want it on the main thread.

I tried to update the function signature, but I get this error, let me know if I misunderstood what you meant. Thanks!

I can go either way then, now that I understand your goal!

In this case you'd need to Dispatch to the main queue to ensure the completion handler is always called on the main thread, that's expected actually. 👍

Ah okay, I wasn't sure if there was a route without dispatching to main, lets leave this as is then. I'll need to revisit this area when I use the new method anyway.

ih-codes · 2025-10-06T17:56:13Z

firefox-ios/Client/Frontend/Browser/SearchEngines/RecentSearchProvider.swift


-/// A provider that manages recent search terms for a specific search engine.
+/// A provider that manages recent search terms from a user's history storage.
 struct DefaultRecentSearchProvider: RecentSearchProvider {


So, the min is used in terms of the date timestamp. Does this mean we're fetching everything from all of time? 🤔

ih-codes · 2025-10-06T17:59:41Z

firefox-ios/firefox-ios-tests/Tests/ClientTests/Search/DefaultRecentSearchProviderTests.swift

+    func test_addRecentSearch_withWhitespaces_trimsAndReturnsValidSearchTerm() {
+        let sut = createSubject(for: "engineA")
+
+        sut.addRecentSearch("   swift  ", url: "https://example.com")
+        sut.addRecentSearch("   ", url: "https://example.com")
+        sut.addRecentSearch("", url: "https://example.com")
+
+        XCTAssertEqual(mockHistoryStorage.noteHistoryMetadataCallCount, 1)
+        XCTAssertEqual(mockHistoryStorage.searchTermList.reversed(), ["swift"])
+    }


ih-codes · 2025-10-06T18:00:47Z

firefox-ios/firefox-ios-tests/Tests/ClientTests/Search/DefaultRecentSearchProviderTests.swift

@@ -0,0 +1,86 @@
+// This Source Code Form is subject to the terms of the Mozilla Public


I know these aren't exactly new, but great test coverage for all the edge cases! 🚀

ih-codes · 2025-10-06T18:03:33Z

firefox-ios/firefox-ios-tests/Tests/ClientTests/Toolbar/ToolbarMiddlewareTests.swift

+    func test_didSubmitSearchTerm_withoutURL_doesNotAddRecentSearchToHistoryStorage() {
+        let subject = createSubject(manager: toolbarManager)
+        let action = ToolbarAction(
+            searchTerm: "cookies",
+            windowUUID: windowUUID,
+            actionType: ToolbarActionType.didSubmitSearchTerm
+        )
+
+        subject.toolbarProvider(mockStore.state, action)
+        XCTAssertEqual(mockRecentSearchProvider.addRecentSearchCalledCount, 0)
+    }
+
+    func test_didSubmitSearchTerm_withoutSearchTerm_doesNotAddRecentSearchToHistoryStorage() {
+        let subject = createSubject(manager: toolbarManager)
+        let action = ToolbarAction(
+            windowUUID: windowUUID,
+            actionType: ToolbarActionType.didSubmitSearchTerm
+        )
+
+        subject.toolbarProvider(mockStore.state, action)
+        XCTAssertEqual(mockRecentSearchProvider.addRecentSearchCalledCount, 0)
+    }


This is exactly the type of weirdness I was describing in the meeting earlier with Redux action payload types, which action creators might solve. I have never liked how our redux actions could be paired with "bad" state.

Nothing actionable here, just pointing out one of my redux gripes. 🤣

Yeah 😅 I agree its the dangerous part of this, so I add tests for now lol.

I remember we used to have our actions as enums and each type had an associate value so we didn't have to have this bad state, but that had its own issues.

Do you remember those issues? Because that's exactly the direction I was thinking we might need to move for the redux actions to have correctly typed action payloads. 🤔

For what I remember, it seem that it was difficult to scale with, an example is when we needed a windowUUID to be associated with each action. Matt may have more insight. Here's a ticket to describe a discussion he had with Orla - https://mozilla-hub.atlassian.net/browse/FXIOS-8188?assignee=625c9c3cd7f1b80069bcb183

Here's the slides on some updates on the changes - https://docs.google.com/presentation/d/1kO25b51gv28zQnos9jrSzyMx4vh9Z4yX4H1jCbXMDM4/edit?slide=id.g1ff410de4db_0_105#slide=id.g1ff410de4db_0_105

Ty for the links @cyndichin, much appreciated! 😄

ih-codes · 2025-10-06T18:06:18Z

firefox-ios/Storage/Rust/RustPlaces.swift

+    /// Currently only used to get the recent searches from the user's history storage.
+    public func getHistoryMetadataSince(
+        since startDate: Int64,
+        completion: @Sendable @escaping (Result<[HistoryMetadata], any Error>) -> Void
+    ) {
+        withReader({ connection in
+            return try connection.getHistoryMetadataSince(since: startDate)
+        }, completion: completion)
+    }
+
+    /// As part of the recent searches work, we are interesting in saving the search term in our history storage.
+    /// - Parameters:
+    ///   - searchTerm: The search term used to find a page.
+    ///   - urlString: The url of the page.
+    public func noteHistoryMetadata(
+        for searchTerm: String,
+        and urlString: String,
+        completion: @Sendable @escaping (Result<(), any Error>) -> Void
+    ) {
+        withWriter(
+            { connection in
+                return try connection.noteHistoryMetadataObservationViewTime(
+                    key: HistoryMetadataKey(
+                        url: urlString,
+                        searchTerm: searchTerm,
+                        referrerUrl: nil
+                    ), viewTime: nil
+                )
+            },
+            completion: completion)
+    }


Thank you for using callbacks and not more Deferred. 🙏

issammani

LGTM ! We can follow-up on using the new method when it's available

cyndichin · 2025-10-07T12:50:07Z

@ih-codes thanks for your comments 🔥🔥 , I updated the PR and responded, ready for another review~

ih-codes

I'll approve now, but you can check my comment response RE: the @MainActor closure for the one method definition.

mergify · 2025-10-08T10:09:36Z

This pull request has conflicts when rebasing. Could you fix it @cyndichin? 🙏

… rebase since URLBarDelegate is deleted

github-actions · 2025-10-08T12:30:59Z

🚀 PR merged to main, targeting version: 144.1

…29797)

cyndichin force-pushed the cc/FXIOS-13499_add-recent-searches-to-Rust-Places branch from 8191858 to af920fb Compare October 3, 2025 16:04

cyndichin commented Oct 3, 2025

View reviewed changes

cyndichin changed the title ~~update~~ Refactor FXIOS-13499 [Trending Searches] recent searches to use A/S Oct 3, 2025

cyndichin force-pushed the cc/FXIOS-13499_add-recent-searches-to-Rust-Places branch from af920fb to e59c1ea Compare October 3, 2025 16:28

cyndichin force-pushed the cc/FXIOS-13499_add-recent-searches-to-Rust-Places branch from e0323e9 to 41cbc75 Compare October 3, 2025 16:41

cyndichin marked this pull request as ready for review October 3, 2025 18:06

cyndichin requested a review from a team as a code owner October 3, 2025 18:06

cyndichin requested review from adudenamedruby, ih-codes, issammani and thatswinnie and removed request for adudenamedruby October 3, 2025 18:06

ih-codes reviewed Oct 6, 2025

View reviewed changes

issammani approved these changes Oct 7, 2025

View reviewed changes

data-sync-user mentioned this pull request Oct 7, 2025

Update fetching history storage to use new method #29858

Open

cyndichin requested a review from ih-codes October 7, 2025 12:49

thatswinnie approved these changes Oct 7, 2025

View reviewed changes

ih-codes approved these changes Oct 7, 2025

View reviewed changes

cyndichin added 6 commits October 8, 2025 08:00

Refactor FXIOS-13499 [Trending Searches] recent searches to use A/S -…

8d632e0

… rebase since URLBarDelegate is deleted

chore: clean up space

cb8713a

enable both FF

0aeed47

revert FF from being on

1949dfd

docs: add todo comment

94c3edd

refactor: move to own private method

f4df765

cyndichin force-pushed the cc/FXIOS-13499_add-recent-searches-to-Rust-Places branch from 3a931ea to f4df765 Compare October 8, 2025 12:01

remove: previously deleted file

ab70f7f

cyndichin merged commit 7ed961c into main Oct 8, 2025
8 of 9 checks passed

cyndichin deleted the cc/FXIOS-13499_add-recent-searches-to-Rust-Places branch October 8, 2025 12:30

cyndichin added a commit that referenced this pull request Oct 8, 2025

Refactor FXIOS-13499 [Trending Searches] recent searches to use A/S (#…

cfec326

…29797)


		finishEditingAndSubmit(searchURL, visitType: VisitType.typed, forTab: tab)

		guard isRecentSearchEnabled else { return }

		@@ -0,0 +1,86 @@
		// This Source Code Form is subject to the terms of the Mozilla Public

Refactor FXIOS-13499 [Trending Searches] recent searches to use A/S #29797

Refactor FXIOS-13499 [Trending Searches] recent searches to use A/S #29797

Uh oh!

Conversation

cyndichin commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Tickets

💡 Description

Testing

📝 Checklist

Uh oh!

cyndichin Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

issammani Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mobiletest-ci-bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💪 Quality guardian

🧩 Neat Piece

💬 Description craftsman

🦊 BrowserViewController Check

❌ Per-file test coverage gate

Client.app: Coverage: 37.39

libStorage.a: Coverage: 56.54

Uh oh!

cyndichin commented Oct 3, 2025

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

issammani left a comment

Choose a reason for hiding this comment

cyndichin commented Oct 3, 2025 •

edited

Loading

cyndichin Oct 3, 2025 •

edited

Loading

issammani Oct 7, 2025 •

edited

Loading

mobiletest-ci-bot commented Oct 3, 2025 •

edited

Loading