Skip to content

feat(transmission): add Transmission torrent client integration - #18

Closed
jeromelefeuvre wants to merge 1 commit into
ndandan:mainfrom
jeromelefeuvre:feature/transmission-integration
Closed

feat(transmission): add Transmission torrent client integration#18
jeromelefeuvre wants to merge 1 commit into
ndandan:mainfrom
jeromelefeuvre:feature/transmission-integration

Conversation

@jeromelefeuvre

Copy link
Copy Markdown

Summary

  • Adds a third torrent-client integration (Transmission) alongside qBittorrent and Deluge, following the same architectural pattern: config storage, health-check/circuit-breaker wiring, route-guard/setup-wizard registration, controller shape, and UI conventions.
  • TransmissionClient speaks Transmission's JSON-RPC protocol over a single /transmission/rpc endpoint, handling its X-Transmission-Session-Id CSRF handshake (an expected 409 is not a failure — the response header carries the token to retry with) and HTTP Basic auth for rpc-authentication-required.
  • TransmissionController mirrors DelugeController's route set (list/detail/actions/bulk/add-by-url/add-by-file/global limits).
  • Wires into HealthService (dashboard health chip + wizard "Test" button, with 409→ok / 401→auth diagnosis), ServiceRouteGuardSubscriber, ConfigExtension, DisplayPreferencesService/DisplayPreferencesExtension (per-page refresh interval), AdminSettingsController (settings fields, kill-switch), SetupController (downloads wizard step), sidebar nav, and EN/FR translations.
  • New transmission/index.html.twig scaffolded from the existing qBittorrent template (table/filters/stat-cards/modals), minus the qBit-only Gluetun/VPN panel.

Test plan

  • docker exec prismarr php bin/console lint:twig — all 169 Twig templates valid
  • docker exec -e APP_ENV=test prismarr php bin/phpunit — 863 tests, 2341 assertions, 0 failures
  • New unit tests: TransmissionClientTest, TransmissionControllerTest, plus additions to ControllersSmokeTest, ServiceRouteGuardSubscriberTest, HealthServiceDiagnoseTest
  • Manual smoke test against a real/dockerized Transmission instance (setup wizard Test button with/without RPC auth, torrent list, pause/resume/delete/recheck, bulk actions, add-by-magnet, add-by-file, global speed limits)

🤖 Generated with Claude Code

New download-client page alongside qBittorrent — setup wizard step,
admin settings, sidebar badge/poll, health circuit breaker — modeled
on the same conventions but with its own JSON-RPC transport, since
Transmission's session handshake (expected HTTP 409 carrying a fresh
X-Transmission-Session-Id to cache and retry with) is genuinely novel
versus qBittorrent's cookie auth.

Also fixes three bugs found while smoke-testing the new client:
- SetupController's Downloads step defaults were non-empty, so
  qBittorrent showed as "configured" even when untouched.
- DashboardController's services-health labels lacked a Transmission
  entry, so its chip never appeared.
- A user-entered URL with a redundant /transmission suffix doubled up
  with the client's own /transmission/rpc path; normalizeBaseUrl()
  strips it.
- torrent-get on a large library (this user has ~1,900 torrents) is
  multiple MB of uncompressed JSON, slow enough over a remote link to
  time out mid-transfer and silently render as "0 torrents". Requesting
  gzip via CURLOPT_ENCODING shrinks that response ~14x.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ndandan

ndandan commented Jul 25, 2026

Copy link
Copy Markdown
Owner

First — this is genuinely well-built. I read TransmissionClient closely and it follows DelugeClient's conventions line for line: the same CURLOPT_PROTOCOLS guard, the same in-process + ServiceHealthCache breaker split, ResetInterface for worker mode, lastError shaped for ApiClientErrorTrait. The 409 session-id handshake is handled correctly (expected first round trip, retried once, doesn't trip the breaker), 401 is correctly diagnosed as auth-not-down, and normalizeBaseUrl() stripping a user-supplied /transmission[/rpc|/web] suffix is a thoughtful touch. The CURLOPT_ENCODING => '' comment about a 6.3MB torrent-get is clearly from real use, not guesswork.

That said, I'm not going to merge it as it stands. The blockers are about what the fork can carry, not about the code:

1. It conflicts. The branch is based on 24c6e77, which predates the UniFi Network tab merge (a7daffe). Both HealthService.php and base.html.twig have moved underneath it — GitHub currently reports the PR as CONFLICTING.

2. 1,967 of the 4,226 lines are transmission/index.html.twig, scaffolded from the qBittorrent template. That's the part I can't take on. It would leave three near-identical torrent templates in the tree (qBit, Deluge, Transmission), and every future fix to the torrent table, filters, stat cards, or modals becomes a three-way edit. If this feature is going to live here, the template needs to be factored into something shared first — that refactor is really the prerequisite work, and it's larger and riskier than the Transmission client itself.

3. The manual smoke test is unchecked, and no CI ran on the branch (gh pr checks reports no checks on feature/transmission-integration), so the 863-test result isn't independently verified here.

4. Strategic: upstream has no Transmission integration, so this is net-new fork-only divergence on top of several existing fork-only features. Deluge is already sitting in Shoshuo#76 waiting on a maintainer decision — I'd rather see how that lands before adding a third client here.

Small unrelated thing while I was reading: the DashboardController.php hunk is a stray blank-line deletion with nothing else in it — worth dropping from the diff.

None of this is a hard no. If you want to take it forward, the order I'd suggest is: rebase onto current main, extract the shared torrent-table template, and get a real smoke test against a live daemon. And if Transmission is something you want in Prismarr generally rather than just in this fork, upstream is the better target — same as you did with Shoshuo#80 and Shoshuo#82.

Either way, thanks for the care that went into this — it's clear you read the codebase rather than pattern-matching at it.

ndandan added a commit that referenced this pull request Jul 26, 2026
Absorbs jeromelefeuvre's Transmission contribution (fork PR #18), rebased
onto post-UniFi main with the contributed client and controller carried as
authored. The page template is rebuilt on the Deluge base plus the shared
partials rather than the contributed qBittorrent-derived copy; qBittorrent
and Deluge are untouched.

Adds guard tests for the registration points and Deluge/Transmission route
parity, plus a render test — nothing in CI previously rendered any torrent
template, because ServiceRouteGuardSubscriber redirects to the setup wizard
under AbstractWebTestCase and the smoke test only asserts status < 500.

956 tests, live-verified on :beta against Transmission 4.1.3.
@ndandan

ndandan commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Landed on main as 79e8ce3. Your commit kept its authorship — git log on main still shows feat(transmission): add Transmission torrent client integration by Jerome Lefeuvre.

First, a correction to my earlier review. I said the template was a blocker because it "would leave three near-identical torrent templates in the tree." That was wrong, and it was the load-bearing part of my objection. templates/deluge/index.html.twig was already a 1,944-line copy of the qBittorrent one — the duplication I was warning you about had been there since the Deluge tab shipped. You weren't introducing a problem; you were following the pattern the repo actually had. Sorry for framing it as something you'd done.

What changed, and why. I rebased aa33ba5 onto post-UniFi main and rebuilt templates/transmission/index.html.twig on the Deluge base rather than the qBittorrent one. Deluge turned out to be the closer semantic match — no Gluetun panel, label-style categories, session-wide pause/resume — and since your TransmissionController declares exactly the same 21 routes as DelugeController, with identical verbs and path parameters, the fetch layer ported by a prefix swap alone. The rebuild also adopts _stat_tiles on top of the three partials Deluge already includes, so the new page shares more than either existing torrent template does.

The translation block went from 208 keys per locale to 15. That follows the Deluge precedent, which reuses the qbittorrent.* namespace and only adds genuinely client-specific strings.

Your client and controller went in as authored, with two exceptions worth naming:

  • DETAIL_FIELDS was missing rateDownload and rateUpload, which getTorrentDetail() reads for properties.dl_speed/up_speed. Since torrent-get only returns requested fields, both silently coalesced to 0 and the detail modal's General tab always showed 0 B/s.
  • The SetupController hunk also blanked the qbittorrent_url/qbittorrent_user wizard prefill defaults, which looked unrelated to Transmission, so I restored those. The stray blank-line-only DashboardController hunk was dropped as you'd expect.

The parts I'd single out as genuinely good, having now read them closely: the 409 handshake is correct and non-obvious, and it works against a real daemon — I verified it live against Transmission 4.1.3. Keeping 401 separate from the breaker matters more than it looks; it's the only thing stopping a wrong RPC password from blackholing the service. And your registration coverage was better than most of my own first passes — you hit every list in HealthService, all four in the settings template, the sidebar and the poller. The only one that moved under you was the turbo:before-render timer array, and only because the UniFi tab landed there after you branched.

Two things your PR indirectly surfaced that were already broken and are now fixed or filed. Nothing in CI ever rendered any torrent template — ServiceRouteGuardSubscriber redirects to the setup wizard under AbstractWebTestCase and the smoke test only asserts status < 500, so the 302 sailed through. There's a render test now. And all three torrent pages leak their 3s poller across Turbo navigations; because the templates share element IDs, a stale qBittorrent interval will write qBittorrent's torrents into the Transmission page's DOM. That one predates your work and is filed separately.

If you do want Transmission in Prismarr generally rather than just this fork, upstream is still the better target — same as you did with Shoshuo#80 and Shoshuo#82. Happy to point at this branch if it helps.

Thanks again. Closing this since the rebase changed the SHA and GitHub won't do it automatically.

@ndandan ndandan closed this Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants