-
-
Notifications
You must be signed in to change notification settings - Fork 284
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
✨ Add Tdarr integration and widget #1882
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi 👋. Thank you for making your first contribution to Homarr. Please ensure that you've completed all the points in the TODO checklist. We'll review your changes shortly.
Thanks a lot for the PR ! I didn't read the code yet but based off the screenshots, have you tried to use the mantine component for scrollarea ? It would remove the ugly automatic chrome scroll bars with better ones when the data overflows |
Ah, good suggestion. I will update the widget. |
https://github.com/Tagaishi/homarr/tree/media-transcode-integration What was missing in my opinion in mine:
|
That would be amazing, thanks for the kind offer. I guess it depends on the use case. I use Tdarr for transcoding audio streams to eliminate the need for on the fly transcoding on devices that don't support Dolby Atmos. So I'm mainly interested in when Tdarr has finished processing a certain file so I can start streaming it. But I also get the appeal of a more minimalist stats widget. I like @ajnart's suggestion to have two separate widgets, or we could create a single one taht does both. Perhaps a Discord poll could be useful for gauging what would be useful for most people? |
Well, it's all yours now, do as you think is best, I trust you ^^ |
src/server/api/routers/tdarr.ts
Outdated
table3Count: z.number(), // Errored Count | ||
}); | ||
|
||
const tdarrNodesSchema = z.instanceof(ZodObject); // equivalent to 'any' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use z.any
? Also is there no way to type this?
src/server/api/routers/tdarr.ts
Outdated
.query(async ({ input }) => { | ||
const app = getTdarrApp(input.appId, input.configName); | ||
|
||
const res = await tdarrApi(app.url, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to response
src/server/api/routers/tdarr.ts
Outdated
mode: 'getById', | ||
docID: 'statistics', | ||
}); | ||
return await res.json(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really have to return all data? Can't you truncate most of it and only return what you need?
src/server/api/routers/tdarr.ts
Outdated
async function tdarrApi(baseUrl: string, bodyData: { | ||
collection: 'StatisticsJSONDB' | 'NodeJSONDB' | 'FileJSONDB' | 'StagedJSONDB', | ||
mode: 'getById' | 'getAll', | ||
docID?: string, | ||
obj?: unknown, | ||
}) { | ||
const url = new URL('/api/v2/cruddb', baseUrl).href; | ||
const options = { | ||
method: 'POST', | ||
headers: { | ||
'Content-Type': 'application/json', | ||
Accept: 'application/json', | ||
}, | ||
body: JSON.stringify({ | ||
headers: { 'content-Type': 'application/json' }, | ||
data: bodyData, | ||
timeout: 5000, | ||
}), | ||
}; | ||
|
||
return await fetch(url, options); | ||
} | ||
|
||
function getTdarrApp(appId: string, configName: string) { | ||
const config = getConfig(configName); | ||
|
||
const app = config.apps.find((x) => x.id === appId); | ||
|
||
if (!app || !checkIntegrationsType(app.integration, ['tdarr'])) { | ||
throw new TRPCError({ | ||
code: 'BAD_REQUEST', | ||
message: `[Tdarr integration] App with ID "${appId}" could not be found.`, | ||
}); | ||
} | ||
|
||
return app; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract to the SDKs
src/widgets/tdarr/TdarrTile.tsx
Outdated
if (!selectedAppId) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to display when not selected?
Also, running hooks conditionally can be dangerous
src/widgets/tdarr/TdarrTile.tsx
Outdated
const app = tdarrApps.find(app => app.id === selectedAppId); | ||
|
||
const [stats, statsStatus, statsError] = useGetTdarrStats(selectedAppId); | ||
// const nodes = useGetTdarrNodes(selectedAppId); // No use for it atm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove it then?
src/widgets/tdarr/TdarrTile.tsx
Outdated
} | ||
|
||
return ( | ||
<Stack spacing="xs" style={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use h
attr
src/widgets/tdarr/api.ts
Outdated
return [{ | ||
queued: result.data?.table1Count, | ||
errored: result.data?.table3Count, | ||
}, result.status, result.error]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fromat? (We use Prettier)
Thanks for the review @manuel-rw. I will try to integrate @Tagaishi's widget in the coming days which will require a good amount of refactoring of my changes anyway, I'll fix your remarks then. |
Did you run formatting on every file in the project? |
babc5ca
to
e9afdac
Compare
Yep, I reverted every change not related to the integration. How do you normally enforce formatting? |
f1 -> format document. (Same for organize imports) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to have widget names that don't harbor the name of the app, in case another integration in the future uses the same widget.
All in all this looks very nice, but very cluttered and heavy.
I would add a modal on each file lines reuniting the infos about it that are not shown otherwise like we do in the torrent widget. This could also allow to remove one or 2 things that may not be strictly needed on the line.
I may have other comments but there's so much I'd need to comb through it a second time.
src/widgets/tdarr/TdarrQueueTile.tsx
Outdated
data: [ | ||
{ | ||
value: 'workers', | ||
label: 'Workers', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove labels to automatically use translations.
src/widgets/tdarr/TdarrQueueTile.tsx
Outdated
appId: app?.id!, | ||
configName: configName!, | ||
}, | ||
{ enabled: !!app?.id && !!configName, refetchInterval: 2000 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't query if the data is not shown on the current page?
src/widgets/tdarr/TdarrQueueTile.tsx
Outdated
return ( | ||
<Stack | ||
justify="center" | ||
style={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use h="100%"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not really a meaningful comment 🤣
src/widgets/tdarr/TdarrQueueTile.tsx
Outdated
height: '100%', | ||
}} | ||
> | ||
<Center style={{ display: 'flex', alignItems: 'center', justifyContent: 'center' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't Center already supposed to align and justify in the center?
Also, you can just use Flex directly iirc.
src/server/api/routers/tdarr.ts
Outdated
|
||
return workers.map((worker) => ({ | ||
id: worker._id, | ||
file: worker.file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend doing the file name shortening here, as you called the value "file" and not "filePath".
(See comment about filename.split('\\').pop().split('/').pop()
)
src/server/api/routers/tdarr.ts
Outdated
jobType: worker.job.type, | ||
status: worker.status, | ||
step: worker.lastPluginDetails?.number ?? '', | ||
originalSize: worker.originalfileSizeInGbytes * 1_000_000, // file_size is in MB, convert to bytes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value is called "originalfileSizeInGbytes", shouldn't the multiplier be for Gbytes then?
Is it giving a value in Mbytes despite having a mention for Gbytes in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to be modifying this file but it's not that important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one slipped through the cracks while I was reverting the formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind the app selector is nice, but I'm not sure it's the place to do it.
First off, this is something to come already in the rework of the integration system, so we don't want to make the users used to a system to change it right after.
Second, You should not have multiple Tdarr instances. That's the whole point of tdarr and node servers. You have one master Tdarr and the nodes connect to it. If that's not the case, I'm sorry to say but you're not using it right.
You can make it autoselect the 1st one, but in any case this will be handled in the future in another way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not be too opinionated about peoples setups, especially seeing as Homarr is an application aimed at users with a certain degree of technical prowess. There might be edge cases or situations in which two Tdarr apps might be viable.
But I will change it so the first app is preselected by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I still mentioned the first point first. Even if people should not do that, in the case they do, it'll be an option in the future.
As it is, it is preferable to not make such a change yet as it is already being worked on.
If you have such ideas that get out of the original scope, you can come to our discord and ask about it.
Wow @Tagaishi you made such a detailed feedback I'm impressed. Sometimes I don't think we should force people to use shorthand properties instead of using style, because both work. But it's good to know that they exist. |
You have Meier and Mani to thank for that x) I always got the same comments on my own PR's because I didn't know the existence of the property. The way mantine decided to shorten them make them really unintuitive. |
That might work for your particular editor and keymap. Could something like a pre-commit hook, lint-staged or a PR check that enforces proper formatting be helpful? |
No idea, never used those. We just manually run "Prettier"'s formatting on files we work on. |
</Group> | ||
<AppSelector | ||
value={value} | ||
onChange={(v) => handleChange(key, v ?? option.defaultValue)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter is the new value, not an event. I'll rename it to value
@jbruell are you still working on this? |
@manuel-rw Sorry, life got in the way a bit. I forgot to backup my Tdarr config before reinstalling my server from scratch and haven't found the motivation to set it up again afterwards. I might spend some time in the coming weeks. I have to sift through Tagaishi's comments again after that and see what I can do with a reasonable time invest. |
I think you covered my comments pretty thoroughly as I saw you thumbed up most if not all of them. (So sorry, I clicked the wrong button...) |
Oh wow new commits ! I need to review them Ping @Tagaishi ;) |
These are my current TODOs which I've extracted from all the comments:
Feel free to add any additional points that I might've missed. |
Sounds good, I think @Tagaishi will be able to give you more insights |
Well, what's stated is all good I think, if you want some input or help on any of it you're welcome to come discuss it all on our discord. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, happy to finally merge this ! Wait for @Tagaishi 's approval just to be sure before merging
@@ -8,6 +8,7 @@ import { | |||
} from '@mantine/core'; | |||
import { Icon } from '@tabler/icons-react'; | |||
import React from 'react'; | |||
import { IntegrationType } from '~/types/app'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that unused ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you've addressed most of my comments, I think we've tinkered with it enough and can tweak it down the line if necessary.
…c270b55 by renovate (#21754) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [ghcr.io/ajnart/homarr](https://togithub.com/ajnart/homarr) | patch | `0.15.2` -> `0.15.3` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>ajnart/homarr (ghcr.io/ajnart/homarr)</summary> ### [`v0.15.3`](https://togithub.com/ajnart/homarr/releases/tag/v0.15.3) [Compare Source](https://togithub.com/ajnart/homarr/compare/v0.15.2...v0.15.3) > \[!NOTE]\ > We've been working actively on working torwards version 1.0 which will include many improvements to performance, security and the overall look & feel of Homarr. It will greatly overhaul the technical architecture of Homarr. This work is done by volunteers. Please consider supporting our work via donations at https://opencollective.com/homarr #### Support for generic Home Assistant switches (lights, fans, ...) Want to toggle your kitchen lights or shut off the music? Using the brand new support for generic switches you can toggle now almost anything: ![homeassistant-lights](https://togithub.com/ajnart/homarr/assets/30572287/2cd79222-ca7d-4773-95e8-845cb382468c) #### Promox integration Homarr now features a Promox widget that can display CPU, RAM and more information directly on your dashboard: ![305290159-b6ed7b8a-335b-41d0-b5c8-8edb736db3ee](https://togithub.com/ajnart/homarr/assets/30572287/cfc6f949-16f8-4add-a4eb-668c258c3305) #### Tdarr widget Homarr now also integrates with Tdarr to display the status of your workers and the queue: ![Untitled](https://togithub.com/ajnart/homarr/assets/30572287/b368c4ae-7b6e-42d7-9bfc-58a110646dbe) #### Add placeholder for dynamic URLs When using advanced reverse proxy setups, sometimes the hostname is not known yet. We have added a new feature that enables you to replace certain placeholders: <img width="598" alt="326171056-a55c3e49-9cd6-4523-90cf-16e91b2c7502" src="https://github.com/ajnart/homarr/assets/30572287/53cf7469-586a-4fda-9dbf-3651cf3e2189"> #### Usability improvements to the torrent widget The torrent widget has received a major upgrade and should scale now better on most devices with improved customizability: ![image](https://togithub.com/ajnart/homarr/assets/30572287/8dbcd786-03ed-49da-b4ef-8a7b5b5bdd46) ![image](https://togithub.com/ajnart/homarr/assets/26098587/325ec664-c0cc-4cfa-bfb5-717d3674bda4) #### Other bugfixes - Fixed overlapping of the grid elements and the navbar - Fixed video playback for iOS and Apple devices - Fixed issues with the certificates for the avatars (this will also resolve issues with the page being shown as "not secure") #### What's Changed - fix: health monitoring hotfix by [@​hillaliy](https://togithub.com/hillaliy) in [https://github.com/ajnart/homarr/pull/1970](https://togithub.com/ajnart/homarr/pull/1970) - fix: change media request TV poster and movie name by [@​SeDemal](https://togithub.com/SeDemal) in [https://github.com/ajnart/homarr/pull/1983](https://togithub.com/ajnart/homarr/pull/1983) - fix: weather widget does not refresh automatically by [@​hillaliy](https://togithub.com/hillaliy) in [https://github.com/ajnart/homarr/pull/1981](https://togithub.com/ajnart/homarr/pull/1981) - fix: [#​1976](https://togithub.com/ajnart/homarr/issues/1976) lower debounce time by [@​manuel-rw](https://togithub.com/manuel-rw) in [https://github.com/ajnart/homarr/pull/1979](https://togithub.com/ajnart/homarr/pull/1979) - chore: new Crowdin updates by [@​ajnart](https://togithub.com/ajnart) in [https://github.com/ajnart/homarr/pull/1968](https://togithub.com/ajnart/homarr/pull/1968) - feat: add Proxmox integration/widget by [@​dslatt](https://togithub.com/dslatt) in [https://github.com/ajnart/homarr/pull/1903](https://togithub.com/ajnart/homarr/pull/1903) - feat: columns customize by [@​hillaliy](https://togithub.com/hillaliy) in [https://github.com/ajnart/homarr/pull/1975](https://togithub.com/ajnart/homarr/pull/1975) - feat: added playsInline to VideoBackground by [@​spkesDE](https://togithub.com/spkesDE) in [https://github.com/ajnart/homarr/pull/1996](https://togithub.com/ajnart/homarr/pull/1996) - fix: ping indicators floating above header in board customization page by [@​krishnamuppaneni](https://togithub.com/krishnamuppaneni) in [https://github.com/ajnart/homarr/pull/1998](https://togithub.com/ajnart/homarr/pull/1998) - fix: OIDC Timeout by [@​catrielmuller](https://togithub.com/catrielmuller) in [https://github.com/ajnart/homarr/pull/2002](https://togithub.com/ajnart/homarr/pull/2002) - fix: Pass axios error into log call for proxmox. Please include in v0.15.3 by [@​dslatt](https://togithub.com/dslatt) in [https://github.com/ajnart/homarr/pull/2012](https://togithub.com/ajnart/homarr/pull/2012) - feat: add romanian language support by [@​Meierschlumpf](https://togithub.com/Meierschlumpf) in [https://github.com/ajnart/homarr/pull/2017](https://togithub.com/ajnart/homarr/pull/2017) - fix: missing romanian language in next-i18next.config.js by [@​Meierschlumpf](https://togithub.com/Meierschlumpf) in [https://github.com/ajnart/homarr/pull/2018](https://togithub.com/ajnart/homarr/pull/2018) - feat: add Tdarr integration and widget by [@​jbruell](https://togithub.com/jbruell) in [https://github.com/ajnart/homarr/pull/1882](https://togithub.com/ajnart/homarr/pull/1882) - feat: torrent widget: polishing UI and improve popover interactions by [@​SeDemal](https://togithub.com/SeDemal) in [https://github.com/ajnart/homarr/pull/2016](https://togithub.com/ajnart/homarr/pull/2016) - fix: Modals titles nested headers and edit mode nested buttons errors by [@​SeDemal](https://togithub.com/SeDemal) in [https://github.com/ajnart/homarr/pull/2019](https://togithub.com/ajnart/homarr/pull/2019) - fix: Avatar host by [@​SeDemal](https://togithub.com/SeDemal) in [https://github.com/ajnart/homarr/pull/2027](https://togithub.com/ajnart/homarr/pull/2027) - feat: Home Assistant entity generic toggle by [@​tuggan](https://togithub.com/tuggan) in [https://github.com/ajnart/homarr/pull/2015](https://togithub.com/ajnart/homarr/pull/2015) - feat: add logout callback URL and session expiration environment variables by [@​SeDemal](https://togithub.com/SeDemal) in [https://github.com/ajnart/homarr/pull/2023](https://togithub.com/ajnart/homarr/pull/2023) - fix: ldap filters by [@​SeDemal](https://togithub.com/SeDemal) in [https://github.com/ajnart/homarr/pull/2033](https://togithub.com/ajnart/homarr/pull/2033) - chore: new Crowdin updates by [@​ajnart](https://togithub.com/ajnart) in [https://github.com/ajnart/homarr/pull/1984](https://togithub.com/ajnart/homarr/pull/1984) - feat: add `[homarr_base]` replacement for external urls by [@​j3lte](https://togithub.com/j3lte) in [https://github.com/ajnart/homarr/pull/2024](https://togithub.com/ajnart/homarr/pull/2024) - core: increase version to 0.15.3 by [@​Meierschlumpf](https://togithub.com/Meierschlumpf) in [https://github.com/ajnart/homarr/pull/2007](https://togithub.com/ajnart/homarr/pull/2007) #### New Contributors - [@​dslatt](https://togithub.com/dslatt) made their first contribution in [https://github.com/ajnart/homarr/pull/1903](https://togithub.com/ajnart/homarr/pull/1903) - [@​krishnamuppaneni](https://togithub.com/krishnamuppaneni) made their first contribution in [https://github.com/ajnart/homarr/pull/1998](https://togithub.com/ajnart/homarr/pull/1998) - [@​catrielmuller](https://togithub.com/catrielmuller) made their first contribution in [https://github.com/ajnart/homarr/pull/2002](https://togithub.com/ajnart/homarr/pull/2002) - [@​jbruell](https://togithub.com/jbruell) made their first contribution in [https://github.com/ajnart/homarr/pull/1882](https://togithub.com/ajnart/homarr/pull/1882) - [@​j3lte](https://togithub.com/j3lte) made their first contribution in [https://github.com/ajnart/homarr/pull/2024](https://togithub.com/ajnart/homarr/pull/2024) **Full Changelog**: ajnart/homarr@v0.15.2...v0.15.3 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNDkuMyIsInVwZGF0ZWRJblZlciI6IjM3LjM0OS4zIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIiwibGFiZWxzIjpbImF1dG9tZXJnZSIsInVwZGF0ZS9kb2NrZXIvZ2VuZXJhbC9ub24tbWFqb3IiXX0=-->
Category
Feature
Overview
Add new integration and widget for Tdarr.
Update 2024-02-13
Here's a short demo video of the latest version of the widget:
homarr-tdarr-demo.mp4
Original comment
The widget uses the Tdarr API (Reference: https://tdarr.readme.io/reference) to fetch and display the following information:
The widget only integrates with one app at a time, but allows the user to choose.
Issue Number
Related issue: #1865
Screenshots
Queued files:
Errors:
Loading skeleton:
Empty queue (with updated header design):
Integration in app settings:
Updated header with app icon & name:
Support for multiple apps: