Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made recent searches be stored per-account #684

Merged
merged 7 commits into from
Oct 4, 2023

Conversation

EricBAndrews
Copy link
Member

@EricBAndrews EricBAndrews commented Oct 1, 2023

Checklist

  • I have read CONTRIBUTING.md
  • I have described what this PR contains
  • This PR addresses one or more open issues that were assigned to me:
    - Slack
  • If this PR alters the UI, I have attached pictures/videos

Pull Request Information

Changed how recent searches are handled persist them per-account. This involves:

  • Changing loadRecentSearches to reloadRecentSearches, which will clear the value and reload using the current account hash (it's only called in one place on .onAppear, so this shouldn't break anything)
  • Persisting recent searches as a map of accountHash to list of ContentModelIdentifier
Recent.Search.Per.Account.PR.mov

@EricBAndrews EricBAndrews requested a review from a team as a code owner October 1, 2023 22:25
@EricBAndrews EricBAndrews requested review from WestonHanners and JakeShirley and removed request for a team October 1, 2023 22:25
@EricBAndrews EricBAndrews enabled auto-merge (squash) October 1, 2023 22:28
Copy link
Collaborator

@mormaer mormaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should be storing an account based value on the APIClient as the client needs to work without accounts too (guest mode).

It's also stored on the client only to be read back out to pass as an argument elsewhere which seems odd.

Would it be better to simply make the recent search tracker / persistence take something like the instance URL as an argument to use as the key?

The communities and user ids will be correct on the same instance for example, even if someone had multiple accounts on the same instance 🤔

Copy link
Collaborator

@mormaer mormaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - there's a test failure for a mismatch of Int instead of String, so I disabled auto-merge in case this approval made it merge when I hit enter 😅

MlemTests/Persistence/PersistenceRepositoryTests.swift Outdated Show resolved Hide resolved
@EricBAndrews EricBAndrews merged commit b24e9ca into dev Oct 4, 2023
4 checks passed
@EricBAndrews EricBAndrews deleted the eric/recents-searches-per-account branch October 4, 2023 00:05
boscojwho added a commit that referenced this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants