feat: new yt channel and instagram ingest#22
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (27)
📒 Files selected for processing (17)
Disabled knowledge base sources:
WalkthroughThe PR adds shared media extraction, YouTube channel bulk ingest, and Instagram ingest flows, expands config and source-kind handling, and updates frontmatter, parser helpers, adapters, and tooling across the repository. ChangesMedia Ingestion and Extraction
Frontmatter and Utility Modernization
Adapter Parser Cleanup
Automation and Review Artifacts
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/adapter/java_adapter_test.go (1)
1071-1101: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winWrap these cases in
t.Run("Should...")subtests.These functions still execute their assertions directly at the top level. Please wrap each scenario in a named subtest to match the repository's required test shape. As per path instructions,
**/*_test.go: MUST uset.Run("Should...")pattern for ALL test cases.Also applies to: 1103-1129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/java_adapter_test.go` around lines 1071 - 1101, The test cases in createJavaResolutionFallbackDiagnosticTruncatesHighVolumeDetailsDeterministically (and the other nearby test scenario in java_adapter_test.go) are still running as top-level assertions instead of named subtests. Wrap each scenario in t.Run with a “Should...” name, keep the existing assertions inside the subtest, and preserve the current setup/expectations around createJavaResolutionFallbackDiagnostic and the truncation checks.Source: Path instructions
internal/cli/inspect_test.go (1)
661-663: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename these subtests to the required
Should...pattern.The loop structure is fine; only the case names are out of policy here. As per path instructions,
**/*_test.go: MUST uset.Run("Should...")pattern for ALL test cases.✏️ Suggested rename
- t.Run(subcommand, func(t *testing.T) { + t.Run("Should show help for "+subcommand, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/inspect_test.go` around lines 661 - 663, The subtests in inspect_test.go are using noncompliant names in the t.Run loop over subcommands; update the subtest naming in the relevant test function to follow the required Should... pattern for every case. Keep the existing loop structure and only change the t.Run case name generation so each subtest under the subcommands slice is named with a Should-prefixed description.Source: Path instructions
internal/mediadl/transcription.go (1)
123-130: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForced STT can currently succeed without a transcriber.
The early return on Line 123 makes the new nil-transcriber branch on Lines 129-130 unreachable when
result.TranscriptionPolicy == TranscriptionPolicySTTandextractor.stt == nil, soExtract(..., {TranscriptionPolicy: STT})can return(result, nil)with no transcript at all.[suggested fix]
Diff
- if !extractor.shouldAttemptSTT(result.TranscriptionPolicy) { + if result.TranscriptionPolicy == TranscriptionPolicyCaptions { return result, transcriptErr } if extractor.ytDLP == nil { return result, errors.Join(transcriptErr, errors.New("media stt: yt-dlp backend is nil")) } if extractor.stt == nil { + if result.TranscriptionPolicy == TranscriptionPolicyAuto { + return result, transcriptErr + } return result, errors.Join(transcriptErr, errors.New("media stt: transcriber is nil")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mediadl/transcription.go` around lines 123 - 130, The early return in Extract prevents the nil-transcriber check from running for forced STT, so a missing transcriber can still return success with no transcript. Update the control flow around shouldAttemptSTT in TranscriptionExtractor.Extract so TranscriptionPolicySTT does not exit before validating extractor.stt. Make the nil-stt branch reachable for forced STT and ensure it returns an error (joined with transcriptErr) instead of a nil error when transcription is requested but unavailable.internal/logger/logger_test.go (1)
31-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the configured level instead of only successful construction.
wantLevelis never checked, so every non-error row passes even ifNewignorestc.leveland always returns the same logger. Emitting a record per case and asserting the"level"field would make this table cover the behavior it names; while touching it, please also switch the subtest names to the requiredShould...form. As per path instructions,MUST test meaningful business logic, not trivial operations,Ensure tests verify behavior outcomes, not just function calls, andMUST use t.Run("Should...") pattern for ALL test cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/logger/logger_test.go` around lines 31 - 47, The logger table test only verifies that New constructs a logger, so it never checks the configured level or catches regressions where tc.level is ignored. Update the logger_test.go cases around New to emit a log record for each tc.level and assert the resulting “level” field matches tc.wantLevel, using the existing tc.name/level table to validate behavior outcomes. Also rename each t.Run subtest to the required Should... form while keeping the same table-driven coverage.Source: Path instructions
🧹 Nitpick comments (8)
internal/ingest/ingest_test.go (1)
745-792: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a small table-driven
Should...matrix here.These cases already share the same setup shape, so a table would remove duplication and align the test with the repo's required subtest pattern. As per coding guidelines,
**/*_test.go: Default to table-driven tests with focused helpers andt.TempDir()for filesystem isolation. As per path instructions,**/*_test.go: MUST uset.Run("Should...")pattern for ALL test cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ingest/ingest_test.go` around lines 745 - 792, The ExistingYouTubeVideoIDs test uses separate ad hoc subtests instead of the required table-driven Should... pattern. Refactor TestExistingYouTubeVideoIDs to use a small table of cases with shared setup/helpers, keep filesystem isolation via t.TempDir(), and rename each t.Run to the required "Should..." format so the two scenarios remain focused but follow the repo test convention.Sources: Coding guidelines, Path instructions
internal/cli/ingest_instagram_test.go (1)
29-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign these CLI tests with the repo test harness conventions.
These scenarios are still one-off top-level tests, and both command invocations hard-code
/tmp/vault. Please move them behindt.Run("Should...")tables/helpers and feed a per-test vault fromt.TempDir()so later filesystem behavior changes do not couple the cases together.As per coding guidelines,
**/*_test.go:Default to table-driven tests with focused helpers and t.TempDir() for filesystem isolation; as per path instructions,**/*_test.go:MUST use t.Run("Should...") pattern for ALL test cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/ingest_instagram_test.go` around lines 29 - 234, The Instagram CLI tests are still standalone top-level cases and they hard-code a shared vault path, which breaks the repo’s test-harness conventions. Refactor TestIngestInstagramCommandComposesCaptionAndIngests and TestIngestInstagramCommandHonorsTranscribeFlag into t.Run("Should...") subtests (ideally table-driven with small shared helpers), and replace the fixed "/tmp/vault" argument with a per-test directory from t.TempDir() so each case is isolated. Keep the behavior assertions the same, but route command setup/execute through reusable helpers to match the existing *_test.go pattern.Sources: Coding guidelines, Path instructions
internal/mediadl/mediadl_test.go (1)
13-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestructure these helper tests into table-driven subtests.
This file is mostly case matrices (
ParseTranscriptionPolicy, language matching, status classification), but every scenario is still a standalone top-level test, and the invalid-policy path only checkserr != nil. Please collapse these into tables behindt.Run("Should...")and assert the expected error text for negative cases so failures stay behavior-specific.As per coding guidelines,
**/*_test.go:Default to table-driven tests with focused helpers and t.TempDir() for filesystem isolation; as per path instructions,**/*_test.go:MUST use t.Run("Should...") pattern for ALL test casesandMUST have specific error assertions (ErrorContains, ErrorAs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mediadl/mediadl_test.go` around lines 13 - 174, Restructure the helper coverage in mediadl_test.go into table-driven subtests using t.Run("Should...") for every case, especially ParseTranscriptionPolicy, languageMatches/normalizeLanguages, and the status-code helpers. Consolidate the standalone test bodies into shared tables with focused helper assertions, and for negative paths like ParseTranscriptionPolicy("maybe") verify the specific error text instead of only checking err != nil. Keep the existing behavior checks tied to the relevant symbols such as ParseTranscriptionPolicy, languageMatches, isStatusCodeNetworkBlocked, and isStatusCodeRateLimited.Sources: Coding guidelines, Path instructions
internal/cli/ingest_test.go (2)
1199-1201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the specific rejection reason here.
This only asserts that some error came back, so an unrelated failure would still satisfy the test. Please assert the invalid-URL diagnostic from
ingest channelas well. As per path instructions,MUST have specific error assertions (ErrorContains, ErrorAs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/ingest_test.go` around lines 1199 - 1201, The test for `ingest channel` only checks that `command.ExecuteContext` returns an error, so it can pass on unrelated failures. Update the assertion in `ingest_test.go` to verify the specific invalid-URL diagnostic from the `ingest channel` path as well, using a targeted error assertion such as `ErrorContains` or `ErrorAs` alongside the existing failure check.Source: Path instructions
1012-1202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap these new command scenarios in
t.Run("Should...")cases.The added channel-ingest coverage is all expressed as standalone top-level tests. Converting them into
Should...subtests would keep this file aligned with the repo’s required test pattern and make shared CLI setup easier to reuse. As per coding guidelines,**/*_test.go:Default to table-driven tests with focused helpers and t.TempDir() for filesystem isolation; and as per path instructions,MUST use t.Run("Should...") pattern for ALL test cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/ingest_test.go` around lines 1012 - 1202, The new channel ingest coverage is currently written as standalone top-level tests instead of the required `t.Run("Should...")` pattern. Wrap `TestIngestChannelCommandBulkIngestsWithResume`, `TestIngestChannelCommandDryRunSkipsIngest`, and `TestIngestChannelCommandRejectsVideoURL` bodies in `t.Run("Should...")` subtests, keeping the shared CLI setup in each test or extracting helpers if needed. Use the existing test symbols like `newRootCommand`, `restoreIngestGlobals`, and the `ingest` command setup to keep the scenarios aligned with the repo’s test structure.Sources: Coding guidelines, Path instructions
internal/youtube/channel_test.go (2)
39-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the invalid-URL contract, not just any error.
The
wantErrbranch only checkserr != nil, so this still passes ifNormalizeChannelURLstarts returning the wrong error kind or message. Please assert*Error/ErrorKindInvalidURLand the expected diagnostic for the rejected input. As per path instructions,MUST have specific error assertions (ErrorContains, ErrorAs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/youtube/channel_test.go` around lines 39 - 47, The invalid-URL test branch in NormalizeChannelURL only checks for a non-nil error, so it should be tightened to assert the specific contract. Update the test in channel_test.go to use the existing ErrorAs/ErrorContains helpers (or equivalent) to verify the returned error is a *Error with ErrorKindInvalidURL, and that it includes the expected diagnostic for the rejected input, rather than accepting any error.Source: Path instructions
15-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign these scenarios with the repo’s required subtest shape.
Most of the new coverage is expressed as standalone top-level tests, and the table-driven case does not use
Should...subtest names. Please fold these scenarios intot.Run("Should...")cases so the file follows the repo’s required test structure consistently. As per coding guidelines,**/*_test.go:Default to table-driven tests with focused helpers and t.TempDir() for filesystem isolation; and as per path instructions,MUST use t.Run("Should...") pattern for ALL test cases.Also applies to: 59-251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/youtube/channel_test.go` around lines 15 - 57, The channel URL normalization coverage needs to follow the repo’s required test naming structure. Update TestNormalizeChannelURL and any related cases to use t.Run with subtest names starting with Should..., keeping the table-driven style and routing each scenario through those subtests. Use the existing NormalizeChannelURL test helper pattern to keep the cases concise while making every test case conform to the required subtest shape.Sources: Coding guidelines, Path instructions
internal/cli/ingest_channel.go (1)
69-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove the bulk-ingest workflow out of the Cobra handler.
This
RunEnow owns normalization, resume filtering, bulk extraction orchestration, per-video ingest, and summary mutation. That makes the command itself the workflow, which is harder to reuse and test than a package-level service/helper. As per coding guidelines,{cmd/kb/**/*.go,internal/cli/**/*.go}:Keep Cobra commands thin and push behavior into internal packages, particularly in internal/cli.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/ingest_channel.go` around lines 69 - 159, The Cobra RunE handler is doing the full ingest workflow instead of delegating it, which makes it hard to reuse and test. Move the normalization, resume filtering, bulk extraction, per-video ingest, and summary mutation out of the command into a package-level helper/service in internal/cli, then keep the RunE function as a thin wrapper that only parses flags, calls the helper, and writes the result. Use the existing symbols like RunE, newYouTubeChannelExtractor, bulk extraction, ingestChannelVideo, and newChannelIngestSummary to locate and extract the workflow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/config_test.go`:
- Around line 325-328: The new empty YouTube caption languages case in Config
validation is too broad because it passes on any error; update the test to
explicitly assert that the failure comes from the youtube.caption_languages
validation branch. In the Config test case around the YouTube caption language
mutation, check the returned error text for youtube.caption_languages so the
test will fail if business logic changes and a different validation path is
triggered.
In `@internal/config/config.go`:
- Around line 220-222: The Validate() flow for YouTube.CaptionLanguages only
trims values to check for emptiness, but leaves the original slice unchanged, so
trimmed languages are not persisted before later use. Update the
validation/normalization logic in config.go so the validated
cfg.YouTube.CaptionLanguages is rewritten with trimmed entries, and make sure
the same normalization is applied in the related YouTube config paths referenced
by Validate() and downstream CLI forwarding so the ingest path receives the
cleaned list.
In `@internal/ingest/youtube.go`:
- Around line 47-50: The frontmatter parsing in the YouTube ingest flow is
silently skipping corrupted documents, which can make already-written videos
look unseen. Update the frontmatter.Parse handling in the ingest logic to
surface the parse failure instead of continuing, using the existing YouTube
ingest path that reads values from the parsed frontmatter. Return or propagate
the error from this branch so resumable ingestion fails loudly on malformed
files rather than reprocessing them.
In `@internal/instagram/instagram_test.go`:
- Around line 74-160: The extractor tests are duplicated standalone cases and
should be consolidated into a table-driven test using the repo’s required
t.Run("Should...") naming pattern. Refactor the scenarios in
TestExtractComposesCaptionAndTranscript, TestExtractTranscriptOnlyWhenNoCaption,
TestExtractCaptionOnlyFallbackWhenTranscriptUnavailable,
TestExtractPropagatesErrorWhenNoCaptionAvailable, and
TestExtractRejectsInvalidURL into one table with focused arrange/act/assert
helpers, keeping the existing Extract and stubCore behavior covered under
separate Should... subtests.
- Around line 148-149: The negative-path checks in instagram_test should assert
the exact error type instead of only checking that an error occurred, so the
tests fail on the wrong error. Update the Extract-based assertions to use
errors.As against the expected mediadl error type and verify the specific
mediadl.ErrorKind value in both failing cases, including the other check at the
additional referenced spot, so the behavior is pinned to the intended
transcript/caption absence path.
In `@internal/instagram/instagram.go`:
- Around line 68-81: Backfill the canonical Instagram URL into Metadata.URL
before returning from the Instagram extraction flow. In the code path around
extractor.core.Extract and the post-processing in
internal/instagram/instagram.go, preserve the normalized URL from
parseInstagramURL and assign it to result.Metadata.URL when yt-dlp does not
provide webpage_url. Make sure this value survives both the successful return
path and the transcript-unavailable fallback so internal/cli/ingest_instagram.go
and internal/ingest/ingest.go always receive a valid canonical source URL.
In `@internal/mediadl/transcription.go`:
- Around line 136-151: The fallback path in transcribeAudioPath currently
preserves transcriptErr when joining audio-download or generic STT failures,
which causes mediadl.IsTranscriptUnavailable to misclassify real
backend/config/network errors. Update the transcript assembly in
transcription.go so only the empty-transcript case carries
ErrorKindTranscriptUnavailable, and ensure audioErr/sttErr failures are returned
without the transcript-unavailable marker; use transcribeAudioPath,
isEmptyTranscriptionError, and the existing ErrorKindTranscriptUnavailable
handling to locate the fix.
In `@internal/mediadl/ytdlp.go`:
- Around line 707-727: The default-caption selection in selectYTDLPCaption is
too dependent on info.Language, so manual subtitles can be missed when yt-dlp
omits language metadata. Update the matching logic in ytDLPCaptionCandidates /
matchingYTDLPCaptionCandidates / ytDLPCaptionLess so a no-preferences request
still accepts usable manual subtitle tracks even when info.Language is empty,
rather than requiring an orig/original classification. Keep the fallback
behavior in selectYTDLPCaption but ensure it can pick a real caption candidate
from subtitles-only payloads like those covered by the ytdlp tests.
In `@internal/youtube/channel.go`:
- Around line 66-72: The host check in NormalizeChannelURL is too permissive
because strings.Contains(host, "youtube.com") accepts lookalike domains. Tighten
the validation in NormalizeChannelURL by checking the parsed host against actual
YouTube hosts only, using host-specific matching for youtube.com and its
legitimate subdomains (and rejecting unrelated domains such as notyoutube.com or
youtube.com.evil.test). Keep the existing ErrorKindInvalidURL path and message
behavior for rejected URLs.
---
Outside diff comments:
In `@internal/adapter/java_adapter_test.go`:
- Around line 1071-1101: The test cases in
createJavaResolutionFallbackDiagnosticTruncatesHighVolumeDetailsDeterministically
(and the other nearby test scenario in java_adapter_test.go) are still running
as top-level assertions instead of named subtests. Wrap each scenario in t.Run
with a “Should...” name, keep the existing assertions inside the subtest, and
preserve the current setup/expectations around
createJavaResolutionFallbackDiagnostic and the truncation checks.
In `@internal/cli/inspect_test.go`:
- Around line 661-663: The subtests in inspect_test.go are using noncompliant
names in the t.Run loop over subcommands; update the subtest naming in the
relevant test function to follow the required Should... pattern for every case.
Keep the existing loop structure and only change the t.Run case name generation
so each subtest under the subcommands slice is named with a Should-prefixed
description.
In `@internal/logger/logger_test.go`:
- Around line 31-47: The logger table test only verifies that New constructs a
logger, so it never checks the configured level or catches regressions where
tc.level is ignored. Update the logger_test.go cases around New to emit a log
record for each tc.level and assert the resulting “level” field matches
tc.wantLevel, using the existing tc.name/level table to validate behavior
outcomes. Also rename each t.Run subtest to the required Should... form while
keeping the same table-driven coverage.
In `@internal/mediadl/transcription.go`:
- Around line 123-130: The early return in Extract prevents the nil-transcriber
check from running for forced STT, so a missing transcriber can still return
success with no transcript. Update the control flow around shouldAttemptSTT in
TranscriptionExtractor.Extract so TranscriptionPolicySTT does not exit before
validating extractor.stt. Make the nil-stt branch reachable for forced STT and
ensure it returns an error (joined with transcriptErr) instead of a nil error
when transcription is requested but unavailable.
---
Nitpick comments:
In `@internal/cli/ingest_channel.go`:
- Around line 69-159: The Cobra RunE handler is doing the full ingest workflow
instead of delegating it, which makes it hard to reuse and test. Move the
normalization, resume filtering, bulk extraction, per-video ingest, and summary
mutation out of the command into a package-level helper/service in internal/cli,
then keep the RunE function as a thin wrapper that only parses flags, calls the
helper, and writes the result. Use the existing symbols like RunE,
newYouTubeChannelExtractor, bulk extraction, ingestChannelVideo, and
newChannelIngestSummary to locate and extract the workflow.
In `@internal/cli/ingest_instagram_test.go`:
- Around line 29-234: The Instagram CLI tests are still standalone top-level
cases and they hard-code a shared vault path, which breaks the repo’s
test-harness conventions. Refactor
TestIngestInstagramCommandComposesCaptionAndIngests and
TestIngestInstagramCommandHonorsTranscribeFlag into t.Run("Should...") subtests
(ideally table-driven with small shared helpers), and replace the fixed
"/tmp/vault" argument with a per-test directory from t.TempDir() so each case is
isolated. Keep the behavior assertions the same, but route command setup/execute
through reusable helpers to match the existing *_test.go pattern.
In `@internal/cli/ingest_test.go`:
- Around line 1199-1201: The test for `ingest channel` only checks that
`command.ExecuteContext` returns an error, so it can pass on unrelated failures.
Update the assertion in `ingest_test.go` to verify the specific invalid-URL
diagnostic from the `ingest channel` path as well, using a targeted error
assertion such as `ErrorContains` or `ErrorAs` alongside the existing failure
check.
- Around line 1012-1202: The new channel ingest coverage is currently written as
standalone top-level tests instead of the required `t.Run("Should...")` pattern.
Wrap `TestIngestChannelCommandBulkIngestsWithResume`,
`TestIngestChannelCommandDryRunSkipsIngest`, and
`TestIngestChannelCommandRejectsVideoURL` bodies in `t.Run("Should...")`
subtests, keeping the shared CLI setup in each test or extracting helpers if
needed. Use the existing test symbols like `newRootCommand`,
`restoreIngestGlobals`, and the `ingest` command setup to keep the scenarios
aligned with the repo’s test structure.
In `@internal/ingest/ingest_test.go`:
- Around line 745-792: The ExistingYouTubeVideoIDs test uses separate ad hoc
subtests instead of the required table-driven Should... pattern. Refactor
TestExistingYouTubeVideoIDs to use a small table of cases with shared
setup/helpers, keep filesystem isolation via t.TempDir(), and rename each t.Run
to the required "Should..." format so the two scenarios remain focused but
follow the repo test convention.
In `@internal/mediadl/mediadl_test.go`:
- Around line 13-174: Restructure the helper coverage in mediadl_test.go into
table-driven subtests using t.Run("Should...") for every case, especially
ParseTranscriptionPolicy, languageMatches/normalizeLanguages, and the
status-code helpers. Consolidate the standalone test bodies into shared tables
with focused helper assertions, and for negative paths like
ParseTranscriptionPolicy("maybe") verify the specific error text instead of only
checking err != nil. Keep the existing behavior checks tied to the relevant
symbols such as ParseTranscriptionPolicy, languageMatches,
isStatusCodeNetworkBlocked, and isStatusCodeRateLimited.
In `@internal/youtube/channel_test.go`:
- Around line 39-47: The invalid-URL test branch in NormalizeChannelURL only
checks for a non-nil error, so it should be tightened to assert the specific
contract. Update the test in channel_test.go to use the existing
ErrorAs/ErrorContains helpers (or equivalent) to verify the returned error is a
*Error with ErrorKindInvalidURL, and that it includes the expected diagnostic
for the rejected input, rather than accepting any error.
- Around line 15-57: The channel URL normalization coverage needs to follow the
repo’s required test naming structure. Update TestNormalizeChannelURL and any
related cases to use t.Run with subtest names starting with Should..., keeping
the table-driven style and routing each scenario through those subtests. Use the
existing NormalizeChannelURL test helper pattern to keep the cases concise while
making every test case conform to the required subtest shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a511e6e-2c45-4241-8254-bf54727bcfa7
⛔ Files ignored due to path filters (29)
.agents/skills/grilling/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/handoff/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/impl-peer-review/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/impl-peer-review/references/impl-review-prompt.mdis excluded by!**/*.md,!.agents/**.agents/skills/impl-peer-review/references/readiness-checks.mdis excluded by!**/*.md,!.agents/**.agents/skills/impl-peer-review/scripts/validate-findings.shis excluded by!.agents/**.agents/skills/kb-yt-channel/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/kb-yt-channel/assets/report-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/kb-yt-channel/references/channel-topic-contract.mdis excluded by!**/*.md,!.agents/**.agents/skills/kb-yt-channel/references/troubleshooting.mdis excluded by!**/*.md,!.agents/**.agents/skills/kb-yt-channel/scripts/ingest-channel.pyis excluded by!.agents/**.agents/skills/spec-peer-review/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/spec-peer-review/references/peer-review-prompt.mdis excluded by!**/*.md,!.agents/**.agents/skills/spec-peer-review/references/quality-markers.mdis excluded by!**/*.md,!.agents/**.agents/skills/spec-peer-review/scripts/validate-findings.shis excluded by!.agents/**.claude/ledger/2026-06-26-MEMORY-instagram-ingest.mdis excluded by!**/*.md,!.claude/**.claude/plans/2026-06-26-kb-ingest-channel.mdis excluded by!**/*.md,!.claude/**.codex/plans/20260626221122-youtube-caption-language.mdis excluded by!**/*.md.peer-reviews/20260627T012748Z/impl-review-findings-round1.mdis excluded by!**/*.md.peer-reviews/20260627T012748Z/impl-review-prompt-round1.mdis excluded by!**/*.md.peer-reviews/20260627T012748Z/impl-review-summary-round1.mdis excluded by!**/*.mdCLAUDE.mdis excluded by!**/*.mdREADME.mdis excluded by!**/*.mdconfig.example.tomlis excluded by!**/*.tomlskills-lock.jsonis excluded by!**/*.jsonskills/kb/SKILL.mdis excluded by!**/*.mdskills/kb/references/architecture.mdis excluded by!**/*.mdskills/kb/references/error-handling.mdis excluded by!**/*.mdskills/kb/references/frontmatter-schemas.mdis excluded by!**/*.md
📒 Files selected for processing (86)
.peer-reviews/20260627T012748Z/impl-review-changed-files-round1.txt.peer-reviews/20260627T012748Z/impl-review-diff-round1.patch.peer-reviews/20260627T012748Z/impl-review-events-round1.jsonl.peer-reviews/20260627T012748Z/impl-review-result-round1.err.peer-reviews/20260627T012748Z/impl-review-status-after-round1.txt.peer-reviews/20260627T012748Z/impl-review-status-before-round1.txtinternal/adapter/go_adapter.gointernal/adapter/java_adapter.gointernal/adapter/java_adapter_test.gointernal/adapter/rust_adapter.gointernal/adapter/treesitter_test.gointernal/adapter/ts_adapter.gointernal/cli/ingest.gointernal/cli/ingest_channel.gointernal/cli/ingest_instagram.gointernal/cli/ingest_instagram_integration_test.gointernal/cli/ingest_instagram_test.gointernal/cli/ingest_test.gointernal/cli/ingest_youtube.gointernal/cli/inspect_helpers_test.gointernal/cli/inspect_test.gointernal/cli/lint_test.gointernal/cli/workflow_test_helpers_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/env.gointernal/convert/csv.gointernal/convert/docx_test.gointernal/convert/epub_test.gointernal/convert/html_test.gointernal/convert/image_common.gointernal/convert/image_test.gointernal/convert/pdf.gointernal/convert/pdf_test.gointernal/convert/pptx_test.gointernal/convert/registry.gointernal/convert/registry_test.gointernal/convert/xlsx_test.gointernal/firecrawl/client_test.gointernal/frontmatter/frontmatter_test.gointernal/generate/benchmark_policy.gointernal/generate/benchmark_policy_test.gointernal/generate/generate_test.gointernal/ingest/ingest.gointernal/ingest/ingest_test.gointernal/ingest/youtube.gointernal/instagram/instagram.gointernal/instagram/instagram_test.gointernal/lint/lint.gointernal/logger/logger_test.gointernal/mediadl/mediadl.gointernal/mediadl/mediadl_test.gointernal/mediadl/openai.gointernal/mediadl/openai_test.gointernal/mediadl/openrouter.gointernal/mediadl/openrouter_test.gointernal/mediadl/transcription.gointernal/mediadl/transcription_test.gointernal/mediadl/ytdlp.gointernal/mediadl/ytdlp_test.gointernal/metrics/compute.gointernal/metrics/compute_test.gointernal/models/kb_models.gointernal/models/kb_models_test.gointernal/models/models.gointernal/output/formatter_test.gointernal/repohealth/portable_paths.gointernal/repohealth/portable_paths_test.gointernal/scanner/scanner_test.gointernal/topic/topic.gointernal/topic/topic_test.gointernal/vault/pathutils_test.gointernal/vault/reader.gointernal/vault/render.gointernal/vault/render_base.gointernal/vault/render_test.gointernal/vault/render_wiki.gointernal/vault/textutils_test.gointernal/vault/writer.gointernal/vault/writer_test.gointernal/youtube/channel.gointernal/youtube/channel_test.gointernal/youtube/youtube.gointernal/youtube/youtube_test.gomagefile.gotest/release_config_test.go
💤 Files with no reviewable changes (25)
- internal/convert/image_test.go
- internal/vault/pathutils_test.go
- internal/convert/pptx_test.go
- internal/generate/benchmark_policy_test.go
- internal/convert/epub_test.go
- internal/output/formatter_test.go
- internal/cli/workflow_test_helpers_test.go
- internal/convert/registry_test.go
- internal/adapter/treesitter_test.go
- internal/repohealth/portable_paths_test.go
- internal/convert/xlsx_test.go
- internal/adapter/rust_adapter.go
- internal/vault/textutils_test.go
- internal/convert/html_test.go
- internal/cli/inspect_helpers_test.go
- internal/scanner/scanner_test.go
- internal/convert/pdf_test.go
- internal/topic/topic_test.go
- test/release_config_test.go
- internal/cli/lint_test.go
- internal/convert/docx_test.go
- internal/adapter/go_adapter.go
- internal/firecrawl/client_test.go
- internal/youtube/youtube_test.go
- internal/adapter/ts_adapter.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
ingest instagramsupport and YouTube channel bulk ingest.--sub-langs/--lang) plus translated-caption controls for media ingestion.