fuck this shitty codebase#657
Conversation
📝 WalkthroughWalkthroughThis PR removes AAC-based download quality options from the UI and refactors the download pipeline to fetch HIGH/LOW requests as LOSSLESS sources, then transcode client-side using new custom AAC format presets instead of server-side transcoding. ChangesAAC Quality Rerouting to Client-Side Transcoding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
|
uwaaa fuck you coderabbit!!!!! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/api.test.ts (1)
119-293:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTests must pass before merge.
The pipeline shows multiple test failures, and the PR description explicitly states the changes are "not tested." This is unacceptable for merge:
Failures directly related to AAC transcoding changes:
- Line 301:
ffmpegcall count mismatch for 'HD Lossless (FLAC)' (expected 1, got 0)- Line 363: File type mismatch for 'HD Lossless (Unchanged)' (expected Mp4File, got FlacFile)
Failures that appear unrelated but need investigation:
- Lines 452, 460, 468: OGG bitrate mismatches (expected vs actual: 314→330, 253→265, 130→137)
- Line 1: RangeError in 'Lossless, but not really' test
Actions required before merge:
- Run the tests in a proper environment (resolve CORS/proxy issues)
- Fix all failing test cases or update expectations with documented justification
- Verify the OGG bitrate mismatches aren't regressions from your changes
- Ensure the 'HD Lossless' test cases work correctly with the new transcoding pipeline
The PR checklist indicates you haven't confirmed local tests pass. Please test thoroughly before requesting review.
🤖 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 `@js/api.test.ts` around lines 119 - 293, Tests fail after AAC transcoding changes: reproduce locally and run the test suite, then fix the transcoding selection and container/bitrate logic so tests expecting ffmpeg calls and file types pass. Inspect Detection (enum), transcodeTrack / chooseTranscode / detectFormat, and the ffmpeg wrapper (where ffmpeg is invoked and ffmpeg call counts are recorded) to ensure preferDolbyAtmos handling still returns TRACK_ATMOS paths, that HD Lossless (FLAC) triggers an ffmpeg call when Detection.FlacHD is detected, and that Mp4File vs FlacFile is produced correctly by the code path that handles Detection.Mp4Flac; also verify OGG encoder bitrate parameters (OGG_320/256/128) in the encoder config are unchanged and adjust them back to prior values if your changes altered the bitrate calculations, and fix the RangeError in the branch for Detection.AacReallyLow by adding proper guards/validation in the code that maps detections to output quality. Ensure all fixes are covered by running the failing tests and update expectations only with documented justification.
🤖 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 `@js/api.test.ts`:
- Around line 408-410: The current AacHigh test asserts a 239 kbps result on
SILENCE_TRACK which may be misleading; add a parallel test that encodes
TRACK_ATMOS (or any non-silent track) with the same AAC settings (e.g., the
existing AacHigh test setup that calls ffmpeg with "-c:a aac -b:a 320k") and
assert the resulting mp4.audioProperties().bitrate is close to the target (e.g.,
within a reasonable tolerance of 320 kbps) to confirm the 239 kbps result is
specific to SILENCE_TRACK rather than a systemic encoder issue; reference the
AacHigh test, SILENCE_TRACK, TRACK_ATMOS, and mp4.audioProperties().bitrate when
locating and adding the new test.
---
Outside diff comments:
In `@js/api.test.ts`:
- Around line 119-293: Tests fail after AAC transcoding changes: reproduce
locally and run the test suite, then fix the transcoding selection and
container/bitrate logic so tests expecting ffmpeg calls and file types pass.
Inspect Detection (enum), transcodeTrack / chooseTranscode / detectFormat, and
the ffmpeg wrapper (where ffmpeg is invoked and ffmpeg call counts are recorded)
to ensure preferDolbyAtmos handling still returns TRACK_ATMOS paths, that HD
Lossless (FLAC) triggers an ffmpeg call when Detection.FlacHD is detected, and
that Mp4File vs FlacFile is produced correctly by the code path that handles
Detection.Mp4Flac; also verify OGG encoder bitrate parameters (OGG_320/256/128)
in the encoder config are unchanged and adjust them back to prior values if your
changes altered the bitrate calculations, and fix the RangeError in the branch
for Detection.AacReallyLow by adding proper guards/validation in the code that
maps detections to output quality. Ensure all fixes are covered by running the
failing tests and update expectations only with documented justification.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71bc9061-a608-4873-b77d-aa3b40ca4315
📒 Files selected for processing (6)
index.htmljs/api.jsjs/api.test.tsjs/download-utils.tsjs/ffmpegFormats.tsjs/settings.js
💤 Files with no reviewable changes (2)
- js/settings.js
- index.html
| // SILENCE_TRACK transcoded with -b:a 320k: ffmpeg's AAC encoder | ||
| // drops the actual bitrate well below the target on silent audio. | ||
| expect(mp4.audioProperties().bitrate).toBe(239); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check FFmpeg AAC encoder configuration
echo "=== FFMPEG_AAC_320 encoder arguments ==="
rg -nP -A5 "FFMPEG_AAC_320" js/ffmpegFormats.ts
echo -e "\n=== FFmpeg invocation in applyAudioPostProcessing ==="
rg -nP -A10 "ffmpeg.*-b:a|ffmpeg.*aac" js/download-utils.tsRepository: monochrome-music/monochrome
Length of output: 422
🏁 Script executed:
# Check the test context around lines 408-410
cat -n js/api.test.ts | sed -n '390,420p'Repository: monochrome-music/monochrome
Length of output: 1916
🏁 Script executed:
# Find SILENCE_TRACK definition
rg -n "SILENCE_TRACK|trackId.*46022548" js/Repository: monochrome-music/monochrome
Length of output: 1105
🏁 Script executed:
# Look for other AAC bitrate test expectations
rg -n "AacHigh|AacLow|bitrate.*toBe" js/api.test.ts | head -20Repository: monochrome-music/monochrome
Length of output: 1055
🏁 Script executed:
# Check git history for the 239 bitrate expectation
git log -p --all -S "toBe(239)" -- js/api.test.ts | head -100Repository: monochrome-music/monochrome
Length of output: 53
🏁 Script executed:
# Search for any discussion or issues about AAC bitrate
rg -i "aac|bitrate|silent" js/ --type-list | head -5 && rg -i "aachigh|codec" js/api.test.ts | head -20Repository: monochrome-music/monochrome
Length of output: 1353
🏁 Script executed:
# Check if there are any comments explaining the SILENCE_TRACK choice
cat -n js/api.test.ts | sed -n '90,110p'Repository: monochrome-music/monochrome
Length of output: 944
🏁 Script executed:
# Compare the pattern: check what bitrate targets are used for each format
rg -n "b:a.*k|bitrate" js/ffmpegFormats.ts | grep -i "aac\|mp3\|vorbis"Repository: monochrome-music/monochrome
Length of output: 616
Investigate AAC 320k bitrate deviation and add validation with non-silent audio.
The AacHigh bitrate expectation of 239 kbps significantly underperforms the 320k target (75% of target). While the comment notes this is due to SILENCE_TRACK, the pattern is concerning:
- AAC 320k: 239 kbps (75% of target) ← outlier
- AAC 256k: 263 kbps (103% of target)
- AAC 96k: 96 kbps (100% of target)
- MP3 320k: 322 kbps (101% of target)
- Ogg 320k: 314 kbps (98% of target)
Only AAC at 320k deviates significantly. The FFmpeg arguments (-c:a aac -b:a 320k) are correct, but this pattern suggests either AAC encoder behavior at 320k specifically differs from other bitrates/codecs, or there's an underlying configuration issue.
Add a test case for AacHigh using TRACK_ATMOS (or another non-silent track) to verify the encoder approaches the target bitrate under normal conditions and confirm the 239 kbps result is specific to silent audio rather than a systemic encoder problem.
🤖 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 `@js/api.test.ts` around lines 408 - 410, The current AacHigh test asserts a
239 kbps result on SILENCE_TRACK which may be misleading; add a parallel test
that encodes TRACK_ATMOS (or any non-silent track) with the same AAC settings
(e.g., the existing AacHigh test setup that calls ffmpeg with "-c:a aac -b:a
320k") and assert the resulting mp4.audioProperties().bitrate is close to the
target (e.g., within a reasonable tolerance of 320 kbps) to confirm the 239 kbps
result is specific to SILENCE_TRACK rather than a systemic encoder issue;
reference the AacHigh test, SILENCE_TRACK, TRACK_ATMOS, and
mp4.audioProperties().bitrate when locating and adding the new test.
1 participant
Description
this shit it not tested cause localhost is blocked by CORS. test this shit!!!!!!!!
Type of Change
Checklist
By submitting this PR, I agree to follow the guidelines. I understand that the final decision to merge rests with the maintainers and that not all contributions can be accepted.
Summary by CodeRabbit