[ptr727]

Summary

Copilot flagged on the 3.23 promotion #1156 that the docs and an implementation comment misdescribe subtitle handling. The comment above ProcessFile.ConvertUsingHandBrake() and ARCHITECTURE.md both listed closed captions among what the no-subtitles HandBrake run handles. That run drops closed captions embedded in the video stream, which #1160 now warns about.

• The ConvertUsingHandBrake() comment says HandBrake drops the embedded closed captions.
• ARCHITECTURE.md says MkvMerge merges back only tracks, so embedded closed captions are lost, and links Docs/ClosedCaptions.md.
• The deinterlace flowchart draws the ReportClosedCaptionsDropped() step that ConvertUsingHandBrake() runs first, with its cancel exit. Repair node R4 names it too.

Docs and one comment only. Docs/ClosedCaptions.md and the 3.23 HISTORY lines already state the drop correctly.

Review

A local strict review pass covers this head. The first pass corrected the stated cause, the link form, and the missing flowchart step. The second found nothing.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 13 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 09317b21-5ad0-463e-813f-c37fbb1a8889

Commits

Reviewing files that changed from the base of the PR and between 65a28c7 and c3833c4.

Files selected for processing: 2

• ARCHITECTURE.md
• PlexCleaner/ProcessFile.cs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[ptr727]

A recorded local strict-review pass covers head a46df23b4e9ed6f46df371e09d75d07e4a8e4998, the content this pull request carries at that commit against develop, and it recorded 0 findings.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Clarify that HandBrake drops embedded closed captions

📝 Documentation 🕐 Less than 10 minutes

AI Description

• Correct documentation and a code comment that implied HandBrake preserves embedded closed
 captions.
• Show the existing caption warning and cancellation path in the deinterlace and repair
 documentation.

Diagram

graph TD
  Caller["HandBrake callers"] --> Check["Caption warning"] -->|continue| HB["HandBrake encode"] --> Tracks{"Subtitle tracks?"}
  Check -->|cancelled| Stop["Stop conversion"]
  Tracks -->|no| Remux["Remux video"] --> Restore["Restore tags"]
  Tracks -->|yes| Merge["Merge subtitles"] --> Restore

Loading

High-Level Assessment

Correcting the architecture guide and adjacent implementation comment is appropriate because the warning and caption loss are already implemented. Changing conversion behavior or duplicating the closed-caption guide would not address the inaccurate descriptions.

Files changed (2) +6 / -4

Documentation (2) +6 / -4

ARCHITECTURE.mdDocument caption loss and the warning in HandBrake flows +5/-3

Document caption loss and the warning in HandBrake flows

• The deinterlace flowchart now shows the existing closed-caption warning and its cancellation exit, and the repair flowchart names the warning. The accompanying text explains that MkvMerge restores separate subtitle tracks but cannot restore captions embedded in video, with a link to the closed-caption guide.

ARCHITECTURE.md

ProcessFile.csCorrect the ConvertUsingHandBrake caption comment +1/-1

Correct the ConvertUsingHandBrake caption comment

• The comment now states that HandBrake drops embedded closed captions rather than converting them to ASS. Executable behavior is unchanged.

PlexCleaner/ProcessFile.cs

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Copilot]

🟡 Changes recommended

An active documentation issue remains because the new cross-reference uses an inline link instead of the repository-required reference-style link with a corresponding definition.

1 open issue

• Use a reference-style link for the documentation cross-reference · New

What changed in this PR

This documentation-only pull request updates the HandBrake processing comment and architecture documentation to explain that no-subtitles rewrites drop embedded closed captions while original subtitle tracks are merged back afterward.

Changes:

• Clarifies ConvertUsingHandBrake() behavior.
• Documents the closed-caption warning and cancellation path.
• Updates the deinterlace flowchart and repair fallback documentation.

Review findings:

• ARCHITECTURE.md: Use a reference-style link for the closed-caption documentation (P2 nit, 2 votes).
• ARCHITECTURE.md: Remove the stale claim that subtitles are converted to ASS (P2 nit, 1 vote).
• PlexCleaner/ProcessFile.cs: Format the explanatory comment as complete sentences (P2 nit, 1 vote).
• PlexCleaner/ProcessFile.cs: Describe the current no-subtitles path instead of claiming subtitles are converted to ASS (P2 nit, 1 vote).

File	Summary
PlexCleaner/​ProcessFile.cs	Updates the HandBrake conversion comment to describe dropped embedded closed captions and the current no-subtitles behavior.
ARCHITECTURE.md	Documents closed-caption loss, warning and cancellation behavior, flowchart changes, and repair fallback handling.

_{Review effort: Lite}

[ptr727]

On the findings list in the Copilot review of a46df23, which open no thread:

ARCHITECTURE.md: Use a reference-style link for the closed-caption documentation

Fixed in c3833c4, which makes every link in ARCHITECTURE.md reference-style.

ARCHITECTURE.md: Remove the stale claim that subtitles are converted to ASS

PlexCleaner/ProcessFile.cs: Describe the current no-subtitles path instead of claiming subtitles are converted to ASS

Fixed in c3833c4. Both now read "HandBrake would convert the subtitles to ASS", which is the reason the run encodes without them.

PlexCleaner/ProcessFile.cs: Format the explanatory comment as complete sentences

Fixed in c3833c4. The comment is two complete lines, and has no trailing period, like the other comments in the file.

[ptr727]

A recorded local strict-review pass covers head c3833c44ad9482b250e674f82c3e61db6745f07f, the content this pull request carries at that commit against develop, and it recorded 0 findings.