Refactor FXIOS-11046 [Homepage Rebuild] [TopSites] Try to improve topsite rendering on the homepage by Cramsden · Pull Request #24222 · mozilla-mobile/firefox-ios

Cramsden · 2025-01-17T19:28:50Z

📜 Tickets

💡 Description

In order to display top sites on the homepage we need to know the NumberOfRows which is configured via a user setting and NumberOfCellsPerRow which is only calculated via the width of the view. It makes sense that NumberOfRows would live on the state since it is configurable with information that is outside of the view, but there is really no reason for NumberOfCellsPerRow to live on the state since it is only a calculation based on information from the view. This PR attempts to simplify that logic so that NumberOfCellsPerRow is removed from the state and actions.

Simulator.Screen.Recording.-.iPhone.16.Plus.-.2025-01-17.at.12.25.46.mp4

📝 Checklist

You have to check all boxes before merging

Filled in the above information (tickets numbers and description of your work)
Updated the PR name to follow our PR naming guidelines
Wrote unit tests and/or ensured the tests suite is passing
When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
If needed, I updated documentation / comments for complex code and public methods
If needed, added a backport comment (example @Mergifyio backport release/v120)

cyndichin · 2025-01-21T14:41:13Z

firefox-ios/Client/Frontend/Home/Homepage Rebuild/HomepageDiffableDataSource.swift

+        let topSites: [HomeItem] = topSitesState.topSitesData.prefix(
+            topSitesState.numberOfRows * numberOfCellsPerRow
+        ).compactMap {
+            .topSite($0, textColor)
+        }


🔥 this makes sense to me to have the calculation here + the previous implementation had a bug

cyndichin · 2025-01-21T14:43:07Z

firefox-ios/Client/Frontend/Home/Homepage Rebuild/HomepageSectionLayoutProvider.swift

        for traitCollection: UITraitCollection,
-        numberOfTilesPerRow: Int
+        size: CGSize,
+        numberOfRows: Int


we are not using numberOfRows here anymore, seems like we can remove this here and where its set in the diffable code

firefox-ios/Client/Frontend/Home/Homepage Rebuild/HomepageSectionLayoutProvider.swift

cyndichin · 2025-01-21T14:47:44Z

firefox-ios/Client/Frontend/Home/Homepage Rebuild/HomepageViewController.swift


    func newState(state: HomepageState) {
-        homepageState = state
+        guard self.homepageState != state else { return }


why do we need to add this check here?

We don't.. We were just playing around to see if it would help cut down on the number of times the data source gets a snapshot applied as an experiment. The homepage newState function gets triggered 30+ times and the snapshot gets applied that many times. the collection view only gets reloaded if the snapshot changes but the collection view sections get recalculated each time and this is a lottt.. I will remove this change now but it is worth more discussion with the team.

yeah :( I agree, let me know if you want me to start that conversation or if you would to! We aren't really doing this in all the other places, so was just curious if there was a reason we added it since I think we were on the path that we were okay with this spamming the calls that may reveal underlying problems.

I removed it. We do this in some places in the codebase but you are right that it is not consistent. We discussed being okay with spamming apply snapshot since it wont actually reload the collectionview unless something has changed but it is concerning to me that we also recalculate the layout many times. 🤷‍♀️ We added this in to see if it would decrease the number of calls as an experiment.

cyndichin · 2025-01-21T14:47:57Z

firefox-ios/Client/Frontend/Home/Homepage Rebuild/HomepageViewController.swift

+        guard self.homepageState != state else { return }
        wallpaperView.wallpaperState = state.wallpaperState
-        dataSource?.updateSnapshot(state: state)
+        dataSource?.updateSnapshot(state: state, numberOfCellsPerRow: numberOfTilesPerRow(for: availableWidth))


I like this a lot better! 🔥

cyndichin · 2025-01-21T14:50:06Z

firefox-ios/Client/Frontend/Home/Homepage Rebuild/TopSites/TopSitesMiddleware.swift


-    private func getTopSitesDataAndUpdateState(for action: Action, and numberOfTilesPerRow: Int? = nil) {
+    private func getTopSitesDataAndUpdateState(for action: Action) {
        Task {


Can you elaborate why we decided to break up into two separate tasks instead of using the task group instead?

This is accomplishing the same behavior but is a bit more straight forward. This is the behavior I believe we expect but let me know if I am wrong!

Fetch top sites

When top sites are fetched update state

fetch sponsored sites

when sponsored sites are fetched update state

If I'm understanding this correctly, this change can lead to a race and stale data since otherSites and sponsoredTiles are both needed for the latest top sites.

For example, the Task to fetch other top sites is using the current sponsored tiles and may not hold the updated one. Vice versa the sponsored tiles are using the original other sites and not the latest one. So could there be inconsistencies in getting the latest sites data from both?

The original code ensures all tasks complete before performing final updates and the final updateTopSites call guarantees that both otherSites and sponsoredTiles have been updated. Let me know if I'm misunderstanding!

ugh ya I see. I will add it back. I think it is unfortunate that we have to fire the same action 3 times but I cant see a way around it right now so I will revert this change.

😓 yeah agreed, thanks!

mergify · 2025-01-23T17:33:13Z

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

cyndichin

thanks for making the updates! I responded to one of your comments, let me know if you need more clarity!

mobiletest-ci-bot · 2025-01-24T02:57:19Z

	Messages
📖	Project coverage: 33.21%
📖	Edited 15 files
📖	Created 0 files

Client.app: Coverage: 32.29

File	Coverage
TopSitesAction.swift	100.0%	✅
HomepageDiffableDataSource.swift	100.0%	✅
TopSitesManager.swift	100.0%	✅
TopSitesMiddleware.swift	100.0%	✅
TopSitesSectionState.swift	96.67%	✅
TopSitesDimensionImplementation.swift	100.0%	✅
HomepageViewController.swift	47.27%	⚠️
HomepageAction.swift	100.0%	✅

Generated by 🚫 Danger Swift against a065161

cyndichin · 2025-01-24T12:14:11Z

...ox-ios-tests/Tests/ClientTests/Frontend/Homepage Rebuild/Redux/TopSitesMiddlewareTests.swift


        let expectation = XCTestExpectation(description: "All relevant top sites middleware actions are dispatched")
-        expectation.expectedFulfillmentCount = 3
+        expectation.expectedFulfillmentCount = 2


should this be 3 instead?

yep! it should be now! and I think there is another test that should be updated to 3 as well. I will make the change!

cyndichin

code looks great now @Cramsden, aside from one minor comment in tests! I pulled the app down from this branch, but getting some issues with top sites... don't think it's your change since I see it in main, but must be something recent, do you see this as well?

This occurs after tapping on a top site.

Screenshot

cyndichin

weird, I reset my simulator and now it works. approving! just that one nit on the tests! thanks for making this improvement 🔥 !!

Cramsden requested a review from a team as a code owner January 17, 2025 19:28

Cramsden requested review from PARAIPAN9 and cyndichin and removed request for PARAIPAN9 January 17, 2025 19:28

cyndichin reviewed Jan 21, 2025

View reviewed changes

firefox-ios/Client/Frontend/Home/Homepage Rebuild/HomepageSectionLayoutProvider.swift Outdated Show resolved Hide resolved

cyndichin reviewed Jan 21, 2025

View reviewed changes

Cramsden requested a review from cyndichin January 22, 2025 19:55

Cramsden force-pushed the cr/FXIOS-11046-top-site-improvements branch from 7fbf234 to 22940f2 Compare January 23, 2025 15:16

Cramsden had a problem deploying to github-pages January 23, 2025 15:16 — with GitHub Actions Failure

cyndichin reviewed Jan 23, 2025

View reviewed changes

Cramsden added 7 commits January 23, 2025 12:18

Remove number of tiles per row from state

4ec68ac

Update tests

92d6b47

Remove unneeded async

6c82cf3

fix tests

d3e6997

update based on feedback

42fd3fc

fix tests and revert syncing changes

313a0fb

Resolve merge conflicts

a76d264

Cramsden force-pushed the cr/FXIOS-11046-top-site-improvements branch from 3c02a10 to a76d264 Compare January 23, 2025 19:29

fix tests

d2528c7

cyndichin reviewed Jan 24, 2025

View reviewed changes

cyndichin approved these changes Jan 24, 2025

View reviewed changes

fix tests

a065161

Cramsden merged commit 65d587c into main Jan 24, 2025
12 of 13 checks passed

Cramsden deleted the cr/FXIOS-11046-top-site-improvements branch January 24, 2025 16:13

Conversation

Cramsden commented Jan 17, 2025

📜 Tickets

💡 Description

📝 Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

mergify bot commented Jan 23, 2025

Uh oh!

cyndichin left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Client.app: Coverage: 32.29

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyndichin left a comment

Choose a reason for hiding this comment

Uh oh!

cyndichin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mobiletest-ci-bot commented Jan 24, 2025 •

edited

Loading