Repository navigation
fix(arr): use authoritative library paths for provider reconciliation - #138
moderniselife merged 1 commit into
Conversation
moderniselife
left a comment
There was a problem hiding this comment.
Thanks @samtruman, another really useful one! Matching existing Radarr/Sonarr entries by TMDb ID before falling back to title matching should make reconciliation a lot more reliable, and centralising the canonical Movies/TV path construction is a nice cleanup.
One thing needs sorting before this can be reviewed properly: the branch looks like it was cut from your fork's integration branch rather than from develop, so the PR currently carries 171 commits and 257 files (+28k lines). That includes dist/ build output, artifacts/media-manager-ux-live/ screenshots, several planning/assessment markdown files, Dockerfile and docker-compose changes, and edits to .github/workflows/ (including a new upstream-update-alert.yml). It also has merge conflicts with develop.
Could you rebase or cherry-pick just the TMDb path-alignment commits onto the current develop, so the diff only contains the Organizer/Arr reconciliation changes and their tests? You mentioned the TypeScript pre-commit check didn't finish, so it'd be great to confirm the type check passes once it's rebased too.
I've passed it to Joe for a look in the meantime. Thanks again for all the work you've been putting into SchroDrive!
|
Thanks for the new commits, @samtruman! The Dolby Vision ranking and season-pack fixes look like useful improvements. Just a heads-up that the branch still carries the full fork history (now 176 commits / 258 files, including dist/, Docker and .github/workflows changes) and still conflicts with develop, so the earlier review still applies. When you get a chance, a branch with only the TMDb/Arr path commits rebased onto develop would make this much easier to review, and the other fixes could come as their own small PRs. Thanks again for all your work on SchroDrive! |
|
Another thing, I would just like to check if you are ready for this PR to come in or if you are still working on it (I can see you adding commits still) If you are ready - I would ask you resolve the conflicts first and I'll merge it in after that. If you arent ready - that's okay, I will add the not ready badge to the PR so I don't merge it in. |
|
If the 176 commits are intentional, we can work together to get those squashed down into a more sensible count for a PR and cleaning up what gets pulled in (IE git ignore the dist and artifacts folders) |
92e1032 to
6fc7cbf
Compare
|
Thanks for flagging this, and apologies for the noisy branch. My fork contains a separate Media Manager subsystem that is not currently part of upstream SchroDrive. I mistakenly reused my production integration branch as the head of this PR. As a result, subsequent Media Manager work — including technical and Dolby Vision ranking, ffprobe comparison evidence, season-pack review logic, aggregate review UI, and deployment artifacts — was unintentionally included in #138. Those changes cannot be reviewed independently in their current form because they depend on the Media Manager subsystem. They should not have been mixed into an upstream PR concerning the existing provider-reconciliation flow. I have now rebuilt #138 from the current upstream
The previous integrated branch has been preserved only as an archive in my PR fork, so no work has been lost, but it is no longer part of this PR. Going forward, any functionality that is genuinely applicable upstream will be proposed separately, based directly on upstream The cleaned branch is ready for review:
|
moderniselife
left a comment
There was a problem hiding this comment.
Thanks so much for rebuilding this, @samtruman, and for the clear write-up! This is now exactly the kind of PR that's easy to review: one commit, three files, based on current develop, no conflicts, and test/guard/GitGuardian are all green.
The change itself reads well. Resolving the existing Radarr/Sonarr entity first and placing the link under Arr's own stored path is a much more robust approach than building a parallel title folder, and matching on original/alternate titles plus the TMDb fallback via Arr's lookup should cut down on misses. Failing closed on ambiguous matches and on paths outside the symlink library is a nice safety touch, and B4-B6 cover the new paths well.
Two small, non-blocking notes for awareness. First, the library-root check compares Arr's stored path directly against symlinkLibraryPath, so it assumes Arr and SchröDrive see that library at the same path. Setups where the containers mount it at different paths will now have items deferred to Review rather than linked. That's the safe outcome, but it might be worth a line in the docs. Second, season folders change from Season 1 to Season 01 (matching Sonarr's default), and shows without a parsed season now link into the series root. Both seem sensible, just flagging them as behaviour changes.
This looks good to me, and it addresses the earlier review. I've passed it to Joe for a final look.
Summary
Scope
This branch is rebuilt from the current
developand is limited to the provider-reconciliation flow already present upstream. It contains one commit and changes only:src/services/providerReconciliation.tstests/e2e/provider-reconciliation-three-inputs.test.tsdocs/provider-reconciliation.mdIt does not contain Media Manager, generated
dist/output, deployment files, Docker changes, workflows, screenshots or fork-specific artifacts.Validation
git diff --check: passed