Skip to content

[ext] feat: keep imported videos in cache to not import them again#2068

Merged
GresilleSiffle merged 6 commits intorate-later-historyfrom
rate-later-history_add_cache
Mar 27, 2025
Merged

[ext] feat: keep imported videos in cache to not import them again#2068
GresilleSiffle merged 6 commits intorate-later-historyfrom
rate-later-history_add_cache

Conversation

@GresilleSiffle
Copy link
Collaborator

@GresilleSiffle GresilleSiffle commented Mar 21, 2025

Description

The history imports made by the browser extension now remembers the YouTube id of imported videos. This allows the users to import new batch of their history daily, without wasting their limited Tournesol API requests by trying to import already imported videos.

To simplify the UI, and to hide the implementation details, the users will see a single number of successfully imported videos, named imported videos:. This number was previously split in two distinct labels new videos and already in your list. A value of 10 means 10 videos from the history are now in your rate later list (some were already present, some are new, but in the end all videos are in the rate-later list).

Preview

During the import 👇️

cap0

When the Tournesol API returns a HTTP 429 response 👇️

capture

Questions

Do we want to allow the users to clear their import cache, to allow them to import a second time their history?

Checklist

  • I added the related issue(s) id in the related issues section (if any)
    • if not, delete the related issues section
  • I described my changes and my decisions in the PR description
  • I read the development guidelines of the CONTRIBUTING.md
  • The tests pass and have been updated if relevant
  • The code quality check pass

@GresilleSiffle GresilleSiffle self-assigned this Mar 21, 2025
@GresilleSiffle GresilleSiffle added the Extension Development of the browser extension label Mar 21, 2025
@GresilleSiffle GresilleSiffle marked this pull request as ready for review March 22, 2025 17:14
@GresilleSiffle GresilleSiffle changed the title [ext] feat: keep improted videos in cache to not import them again [ext] feat: keep imported videos in cache to not import them again Mar 22, 2025
@sigmike
Copy link
Collaborator

sigmike commented Mar 25, 2025

Looks good, works well.

I think the text "You'll be able to start a new import in 24 hours, after having watched new videos" would be better positioned inside the "alert-box". It also probably needs updating (EN and FR) since it's now also valid to start it again the next day even if you haven't watched any new video. Maybe something like "You'll be able to start a new import in 24 hours to continue importing your previous history, or to import any videos that you've watched in the meantime."

The English title of the status box "Videos are being imported, please wait" needs to be updated as it remains there when the import is stopped.

Do we want to allow the users to clear their import cache, to allow them to import a second time their history?

The user can remove and reinstall the extension to clear the local storage. It's probably good enough.

Comment on lines +208 to +211
for (const video of importedVideosSet) {
if (!historyInStorage?.includes(video)) {
videosStr += `,${video}`;
}
Copy link
Member

Choose a reason for hiding this comment

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

Loading youtubeHistoryImported in a Set could perform better, instead of using String.includes(). I suppose that is not issue even for thousands of videos?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I thought about it. I tried with a string of 10,000 videos and the performance were good enough, but it's probably better to use a Set.

@GresilleSiffle GresilleSiffle merged commit 746985a into rate-later-history Mar 27, 2025
6 checks passed
@GresilleSiffle GresilleSiffle deleted the rate-later-history_add_cache branch March 27, 2025 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Extension Development of the browser extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants