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

feat: Support aria2 in download widget #2226

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

hotrungnhan
Copy link

@hotrungnhan hotrungnhan commented Feb 3, 2025


Homarr

Thank you for your contribution. Please ensure that your pull request meets the following pull request:

  • Builds without warnings or errors (pnpm buid, autofix with pnpm format:fix)
  • Pull request targets dev branch
  • Commits follow the conventional commits guideline
  • No shorthand variable names are used (eg. x, y, i or any abbrevation)

Change Logs:

  • Support Aria2 in download widget.
  • Download item detail:
    • Conditional render upspeed, sent and ratio to torrent's item only.
  • Adjust the DownloadClientStatus interface to support the multiple download type client such aria2.

Limitations:

  • Remove operation does not remove downloaded files (Aria2 RPC does not support this).
  • The waiting and finished entries only listing first 1000 items.
  • Can only pause downloading and leeching.
  • The 'error' and completed entries can only be removed.

@hotrungnhan hotrungnhan requested a review from a team as a code owner February 3, 2025 06:13
Copy link

deepsource-io bot commented Feb 3, 2025

Here's the code health analysis summary for commits 7a8e671..799f0cc. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource JavaScript LogoJavaScript✅ SuccessView Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

@hotrungnhan hotrungnhan force-pushed the feature/intergrate_aria2 branch from ae40bfc to 64b6214 Compare February 3, 2025 06:21
@hotrungnhan hotrungnhan force-pushed the feature/intergrate_aria2 branch from 64b6214 to fea3ba4 Compare February 3, 2025 06:22
@hotrungnhan hotrungnhan changed the title feat: Integrate Aria2 feat: Support aria2 in download widget Feb 3, 2025
@manuel-rw manuel-rw added enhancement New feature or request integration New integration labels Feb 3, 2025
aria2: {
name: "Aria2",
secretKinds: [["apiKey"]],
iconUrl: "https://cdn.jsdelivr.net/gh/homarr-labs/dashboard-icons@master/png/aria2.png",
Copy link
Member

Choose a reason for hiding this comment

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

This gives me Package size exceeded the configured limit of 50 MB. Try https://github.com/homarr-labs/dashboard-icons/tree/master/png/aria2.png instead.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the icon does not exist and has to be added to the dashboard-icons repository

Copy link
Author

@hotrungnhan hotrungnhan Feb 5, 2025

Choose a reason for hiding this comment

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

Aria2 don't have any offical icon. I found in the internet the icon below, or if you find any better, please help add it to the dashboard-icons repository.
aria2

name: "Aria2",
secretKinds: [["apiKey"]],
iconUrl: "https://cdn.jsdelivr.net/gh/homarr-labs/dashboard-icons@master/png/aria2.png",
category: ["downloadClient", "torrent", "http(s)"],
Copy link
Member

Choose a reason for hiding this comment

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

I dislike that the category has brackets in it's name. Just a suggestion: multiProtocolDownloadClient?
@Meierschlumpf / @SeDemal WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

From Aria2's main page: "It supports HTTP/HTTPS, FTP, SFTP, BitTorrent and Metalink"
Since it supports more than http(s), and this client is the only one using all the different kinds, I suggest to just call it "miscellaneous" or something like that. All in all though, it actually boils down to the type of exchange which is one way or both ways, so http is of the same type as usenet downloads.

Copy link
Author

Choose a reason for hiding this comment

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

updated to use "miscellaneous"

Copy link
Member

Choose a reason for hiding this comment

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

Why an empty file?

@manuel-rw manuel-rw requested a review from a team February 3, 2025 21:33
@SeDemal
Copy link
Collaborator

SeDemal commented Feb 4, 2025

I'll need to spend some more time later on for a proper review.
Few things to note though:

  • Properly check all the tests (pnpm -> fomrat:fix, lint, typecheck)
  • Make the integration fit the current template as much as possible (I'm unsure if it's relevant here, just a reminder)
  • If you're going to make your own client interpretation of the dependency, then why not get rid of it? It seems to me that a http RPC function should be fairly easy here from what I see in the docs. (nzbGet uses RPC as well, if you want a template)
  • Use the proper fetch function, we use "fetchWithTrustedCertificatesAsync" because of certificates.

@hotrungnhan
Copy link
Author

hotrungnhan commented Feb 6, 2025

@SeDemal @manuel-rw @Meierschlumpf finished. can you help review again ?

is there existing way to disable the action button base on download item state ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request integration New integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants