fix(translate): name the media in translation job labels for movies - #427
Conversation
The manual translate action on a movie produced a job called "Translating EN to HU" with no film in it, and the AI Translator jobs table showed only the language pair. Episodes were fine. get_title and get_description branch on media_type, and the callers spell it two different ways: the translate endpoint sends "movie" while the batch and mass-operation paths send "movies". Only the plural was matched, so a movie fell through to the series branch, found no sonarr_series_id and returned an empty string. That emptied the job label, the Media column, and the overview the AI translator puts in its prompt, which quietly cost movies their context. Both functions now accept either spelling through one predicate, so a caller cannot lose the title again by picking the other word.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62c5f38458
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) -> str: | ||
| try: | ||
| if media_type == "movies": | ||
| if is_movie_media_type(media_type): |
There was a problem hiding this comment.
When the manual endpoint supplies media_type="movie" in a multi-Radarr installation, this newly entered branch looks up the title solely by radarrId, even though that ID is only unique together with arr_instance_id. If two instances have the same ID, .first() can select the sibling movie, producing the wrong job label and sending the wrong title—and, through the equivalent get_description branch, overview—to the translation provider. Thread the already-available arr_instance_id into these helpers and include it in both movie queries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 43ff351. ux_table_movies_instance_upstream_id does make radarrId unique only per instance, and the same holds for sonarrSeriesId, so the lookup could return a sibling instance's media. You are also right that this branch is what makes it reachable: before the spelling fix the movie branch was dead code for the manual endpoint.
Both helpers now take arr_instance_id and narrow every query with it, the episode lookup included, since TableEpisodes carries the column too. The owner is threaded from translate_subtitles_file, which already received it, through the factory into all four translator services. A caller with no owner passes None and behaves as before, which keeps the batch and mass-operation paths unchanged.
Tests seed two instances that reuse the same upstream ids and assert each instance gets its own film, episode and overview, plus three cases that the owner actually survives the trip through the services and the job label, since threading is where this kind of fix leaks. Backend group 978 passed. Re-verified live on the test server: translating a movie produced Translating 12 Angry Men (EN to HU) with the sidecar job carrying title='12 Angry Men'.
…ance radarrId and sonarrSeriesId are unique only together with arr_instance_id, so looking a title up by the upstream id alone can return a sibling instance's media. With two Radarr instances holding the same id, a translation could be labelled with the wrong film and, through get_description, hand the wrong overview to the provider as prompt context. Both helpers now take the owning instance and narrow every query with it. The owner is threaded from translate_subtitles_file, which already had it, through the factory into the translator services, so the label and the prompt describe the media the user actually asked about. Callers with no owner pass None and behave as before. The movie branch was unreachable from the manual endpoint until the media_type spelling fix in this branch, so this closes the hole that fix would otherwise have opened.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What
A movie translated from its own page produced a job called
Translating EN to HU, with no film named, and the AI Translator jobs table showed only the language pair in its Media column. Episodes were unaffected.Translating EN to HUTranslating 12 Angry Men (EN to HU)(ENGLISH → HUNGARIAN)12 Angry Men (ENGLISH → HUNGARIAN)Why
get_titleandget_descriptionboth branch onmedia_type, and the callers do not spell it the same way:moviemoviesmoviesOnly the plural was matched. A movie therefore fell through to the series branch, found no
sonarr_series_id, and returned an empty string. That emptied three things at once: the job label, the Media column in the jobs table, and the movie overview that the AI translator puts into its prompt. The last one is the least visible and arguably the worst, since it costs every directly translated movie its context.Both functions now go through one
is_movie_media_typepredicate that accepts either spelling, so a caller cannot lose the title again by choosing the other word. Fixing the predicate rather than the one call site also coversget_description, which had the same fault.Instance scoping
Reaching the movie branch exposed a second problem, raised in review.
radarrIdandsonarrSeriesIdare unique only together witharr_instance_id, asux_table_movies_instance_upstream_idsays, so an unscoped lookup can return a sibling instance's media. Two Radarr instances holding the same id would label a translation with the wrong film and hand the wrong overview to the provider as prompt context.Both helpers now take the owning instance and narrow every query with it, including the episode lookup. The owner is threaded from
translate_subtitles_file, which already received it, through the factory into the translator services. Callers with no owner pass None and behave exactly as before.Verification
New
tests/bazarr/test_translate_job_media_name.py, registered inci.yml. It seeds two instances that reuse the same upstream ids, then asserts the title and description resolve under both spellings, that each instance gets its own film and episode, that an unknown movie or an instance owning nothing still yields an empty title, and that the episode label keeps itsS02E05form. Before the spelling fix the twomoviecases failed and everything else passed, which is the bug exactly; before the scoping fix eight cases failed.Three further cases assert the owner survives the trip: each translator service stores the
arr_instance_idit was constructed with, andtranslate_subtitles_filepasses it to the lookup behind the job label. Threading is where a fix like this usually leaks.Backend CI group covering the translator: 978 passed.
Deployed to the test server and translated a real movie subtitle from the movie page. The job read
Translating 12 Angry Men (EN to HU)and the sidecar job carriedtitle='12 Angry Men', where the jobs queued before the fix still show an empty title.