Don't make requests when importing subscriptions #5617
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
PR: waiting for review
ChunkyProgrammer
requested review from
PikachuEXE,
absidue,
efb4f5ff-1298-471a-8973-3d47447115dc and
kommunarr
|
Hi error~ |
absidue
reviewed
Aug 28, 2024
absidue
previously approved these changes
Aug 28, 2024
|
Oops more errors |
|
@PikachuEXE found the issue, interestingly, this error could happen when not using this PR |
ChunkyProgrammer
force-pushed
the
dont-make-requests-when-importing-subscriptions
branch
from
ee4aff6 to
91ef230
Compare
|
|
@PikachuEXE you're somehow subscribed to the same channel multiple times, not sure how that happened |
|
I think the double subscribe issue should be fixed here: 60a11d3 (you'll need to unsubscribe from the duplicate manually) |
absidue
approved these changes
Aug 31, 2024
There was a problem hiding this comment.
Looks good to me now. There are a few things that would be nice to have but are non-blocking:
- It's probably not worth keeping the progress bar logic anymore as the import is so fast now that there is probably more overhead in updating the progress than in the processing itself (additionally the progress bar has a 500ms animation, so when the import finishes in less than 60ms the progress bar will visually probably only reach 10% before we hide it).
- I was premature in suggesting that the
asynckeyword should be removed, it's probably worthawaiting thethis.updateProfile()calls as they are asynchronous, so we only show theAll subscriptions have been successfully importedtoast once we know that the data has actually been saved. (if this is done, I would suggest adding back theThis might take a while, please waittoast in all places that it was removed)
efb4f5ff-1298-471a-8973-3d47447115dc
approved these changes
Sep 1, 2024
PikachuEXE
approved these changes
Sep 2, 2024
github-actions
bot
removed
the
PR: waiting for review
ChunkyProgrammer
deleted the
dont-make-requests-when-importing-subscriptions
branch
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Don't make requests when importing subscriptions
Pull Request Type
Related issue
Description
A simplified version of #4340 that doesnt worry about having a lot of logic for fetching thumbnails if it's missing, the thumbnails can be fetched from other places (visiting the channel, refreshing subscriptions with local api + no rss)
Screenshots
Testing
See: #3735 (comment) & #4340
Change file extension
.txtto.db(Notice how much faster importing subscriptions is)
Desktop