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.

Sign up for GitHub

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

Jump to bottom

Download completed callback improvement #1617

Merged
merged 2 commits into from
Feb 24, 2025
Merged

Download completed callback improvement #1617

merged 2 commits into from
Feb 24, 2025

Conversation

VilppeRiskidev
Copy link
Collaborator

No description provided.

@VilppeRiskidev VilppeRiskidev self-assigned this Jan 14, 2025
@VilppeRiskidev VilppeRiskidev changed the base branch from develop to simplify-download-monitor January 14, 2025 19:03
@VilppeRiskidev
Copy link
Collaborator Author

See also: https://github.com/thunderstore-io/thunderstore-mod-manager/pull/297

Base automatically changed from simplify-download-monitor to develop January 15, 2025 06:40
anttimaki
anttimaki reviewed Jan 15, 2025
View reviewed changes
@VilppeRiskidev VilppeRiskidev force-pushed the download-completed-callback-improvement branch 2 times, most recently from 0ddd566 to 8080030 Compare January 16, 2025 13:17
@VilppeRiskidev VilppeRiskidev marked this pull request as ready for review January 16, 2025 13:18
@VilppeRiskidev VilppeRiskidev force-pushed the download-completed-callback-improvement branch from 8080030 to 53c1621 Compare January 17, 2025 13:19
@VilppeRiskidev VilppeRiskidev deleted the download-completed-callback-improvement branch January 21, 2025 12:47
@VilppeRiskidev VilppeRiskidev restored the download-completed-callback-improvement branch January 21, 2025 12:50
@VilppeRiskidev VilppeRiskidev force-pushed the download-completed-callback-improvement branch from 53c1621 to e433150 Compare January 21, 2025 13:45
anttimaki
anttimaki requested changes Jan 23, 2025
View reviewed changes
Copy link
Collaborator

@anttimaki anttimaki left a comment

Choose a reason for hiding this comment

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

This passes the "review" state, but on manual testing, it seems that errors don't properly set the state of the download tracker. I.e:

  1. Start download
  2. Before it finishes, use dev tools to go offline
  3. Error modal is shown
  4. In the DownloadMonitor view, the download is still shown as in progress

I think the catch-block in downloadHandler should call Vuex's download/updateDownload with failure state, but do test this and other error cases thoroughly yourself too.

@VilppeRiskidev VilppeRiskidev force-pushed the download-completed-callback-improvement branch from e433150 to 911baa4 Compare January 23, 2025 14:44
@VilppeRiskidev
Copy link
Collaborator Author

This passes the "review" state, but on manual testing, it seems that errors don't properly set the state of the download tracker. I.e:

  1. Start download
  2. Before it finishes, use dev tools to go offline
  3. Error modal is shown
  4. In the DownloadMonitor view, the download is still shown as in progress

I think the catch-block in downloadHandler should call Vuex's download/updateDownload with failure state, but do test this and other error cases thoroughly yourself too.

Should be fixed now. The progress failed status is now updated when the download fails.

Displaying the error in DownloadMonitor will be improved in the future.

anttimaki
anttimaki requested changes Jan 24, 2025
View reviewed changes
Copy link
Collaborator

@anttimaki anttimaki left a comment

Choose a reason for hiding this comment

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

image

As discussed in yesterday's daily, this is not what we want. This should unambiguously show the download failed. Even if the improvements we discussed are done separately, this here is a bug that should've been caught in your testing.

@VilppeRiskidev VilppeRiskidev force-pushed the download-completed-callback-improvement branch from 5242ff6 to 9ca4173 Compare January 28, 2025 12:59
anttimaki
anttimaki requested changes Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@anttimaki anttimaki left a comment

Choose a reason for hiding this comment

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

image

I think it's worse now (this is a failed download). How are you testing this? Maybe there's a difference between errors?

@VilppeRiskidev VilppeRiskidev force-pushed the download-completed-callback-improvement branch 2 times, most recently from db6a4f9 to 7ea20ca Compare January 30, 2025 14:05
@VilppeRiskidev
Copy link
Collaborator Author

At least I can't get it to fail without it showing the correct error state in ModDownloadMonitor anymore.

anttimaki
anttimaki approved these changes Feb 3, 2025
View reviewed changes
Copy link
Collaborator

@anttimaki anttimaki left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Since this is now working as expected I'm approving it, but please ensure the newest comments are addressed in #1625 and on a separate task. Note that this makes merging this branch sort of conditional on having #1625 merge-ready.

@VilppeRiskidev VilppeRiskidev force-pushed the download-completed-callback-improvement branch from 7ea20ca to 9ca4173 Compare February 3, 2025 13:48
@anttimaki anttimaki merged commit ad85719 into develop Feb 24, 2025
9 checks passed
@anttimaki anttimaki deleted the download-completed-callback-improvement branch February 24, 2025 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anttimaki anttimaki anttimaki approved these changes

Assignees

@VilppeRiskidev VilppeRiskidev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@VilppeRiskidev @anttimaki