[Bug]: Recently saved section should not show items older than 10 days

[csadilek]

Steps to reproduce

1. Open Fenix
2. "Recently Saved" section displays bookmarks saved a long time ago as we currently just query the n (4) most recently saved bookmarks but we're not filtering based on time. There's no cut-off for "recently" :).

Expected behaviour

"Recently Saved" should only show items created in the last 10 days.

Actual behaviour

"Recently Saved" shows the last n (4) saved bookmarks, independently of when they were created.

Device name

All

Android version

All

Firefox release type

Firefox Nightly

Firefox version

92

Device logs

No response

Additional information

No response

┆Issue is synchronized with this Jira Task

[mcarare]

a-s ticket: mozilla/application-services#4349

[csadilek]

@mcarare I don't think we need A-S for that, do we? A-S is already returning the most recently added bookmarks in descending order (most recent first). We can just filter by dateAdded on our end. Even if we fetched more it wouldn't matter because they would be before our cut-off date. In other words if we fetch 4 bookmarks and then filter out 2, it would not matter as there are no newer ones anyway. So, it should be fine to do on our end, A-C or Fenix.

The only thing we'd have to do is expose the dateAdded property which is in the A-S Kotlin class but not it A-C:
A-S: https://github.com/mozilla/application-services/blob/72b827c3e0f883163762857fd766df1aeb060725/components/places/android/src/main/java/mozilla/appservices/places/Bookmarks.kt#L86

A-C (conversion where we drop dateAdded): https://github.com/mozilla-mobile/android-components/blob/6c0fe91ab83614265f533b819bf406fc046b2227/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt#L94

[csadilek]

I mean it's fine if we want to add this to A-S, but we shouldn't be waiting on it :). I think either A-C or A-S are fine for us to implement this in, but would vote to just add to A-C thinking about it more.

[mcarare]

@csadilek I would have prefered to fetch the list and filter it in just one place(a-s) rather than filtering in once in a-s and then in a-c/ Fenix while also needing to expose another property we do not currently need elsewhere.

[csadilek]

@mcarare Yes, thanks! I mostly didn't think we should be waiting here. We can open a PR against A-S too! However, since A-S isn't actually filtering right now and we don't know exactly what we want until we get some user feedback and run experiments, it makes sense to implement this on our end first before adding more API surface to A-S, esp. as it's easy to do. But as I wrote, I think it's fine either way. :)

[csadilek]

Ah and to clarify "we don't know exactly what we want". We're also still discussing if this feature should instead be based on most used or most recently used bookmarks. Any change here would mean we need different API from A-S, that's what I meant with let's wait until we know more :). You're definitely right though that ultimately we want this in A-S so that we can use it in iOS as well.

[csadilek]

This is fixed now! Thanks @mcarare.