Conversation
WalkthroughSignificant refactor of Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Load
participant Init as init()
participant Theme as setupTheme()
participant HW as detectHardware()
participant Menu as Menu Handlers
participant Compress as Compression Flow
App->>Init: Window load
Init->>Theme: setupTheme()
Note over Theme: Read localStorage,<br/>apply theme
Init->>HW: detectHardware()
Note over HW: GPU detection,<br/>platform checks,<br/>showOptions()
rect rgb(220, 240, 255)
Note over Menu: Event Wiring
Menu->>Menu: toggleMenu()
Menu->>Menu: closeMenu()
Menu->>Menu: navigateTo()/navigateToWin()
end
rect rgb(240, 220, 255)
Note over Compress: Compression Workflow
Compress->>Compress: startCompression()
Compress->>Compress: buildFFmpegCommand()
alt Success
Compress->>Compress: handleCompressionSuccess()
else Error
Compress->>Compress: handleCompressionError()
end
Compress->>Compress: updateProgress()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/compressor.js (3)
100-123: Cancellation vs error handling can double‑report cancelled jobsThe refactored
startCompression()loop is clean, but the interplay with cancellation is a bit off:
cancelCompression()marksisCancelled = trueand immediately callsupdateProgress("error", "Cancelled", currentItemId).- When ffmpeg exits due to the injected
"q",compressVideowill likely reject, and thecatchinstartCompression()then callshandleCompressionError(itemId, error), which overwrites the previous “Cancelled” state and appends an error element.This means a user‑initiated cancel is surfaced as an error with “click for details” instead of remaining a clean “Cancelled” state.
Consider skipping
handleCompressionErrorwhenisCancelledis true, e.g.:- } catch (error) { - handleCompressionError(itemId, error); - } + } catch (error) { + if (!isCancelled) { + handleCompressionError(itemId, error); + } + }(And possibly reset
isCancelledin the cancel path once all processes have exited.)
134-183: Guard progress calculation against zero/unknown durationNice touch adding the
if (!timeStr) return 0;guard intimeToSeconds. However, incompressVideoyou still compute:const totalSeconds = timeToSeconds(video_info.duration); ... const progress = Math.round((currentSeconds / totalSeconds) * 100);If
time=is logged beforeDuration:(orDurationhas not yet been parsed),video_info.durationis empty,totalSecondsbecomes0, and you end up dividing by zero, yieldingInfinityand possibly writingvalue=Infinityto the<progress>element.Recommend checking
totalSeconds > 0before computing progress:- const totalSeconds = timeToSeconds(video_info.duration); + const totalSeconds = timeToSeconds(video_info.duration); ... - if (currentSeconds && !isCancelled) { + if (currentSeconds && totalSeconds > 0 && !isCancelled) { const progress = Math.round( (currentSeconds / totalSeconds) * 100 );This makes progress updates robust even when ffmpeg output arrives in an unexpected order.
Also applies to: 498-498
204-339: Fixvideotoolboxargument order, wrap AMFconstdeclarations in blocks, and addvideoQualityvalidationThree issues confirmed in
buildFFmpegCommand:
Critical:
videotoolboxarguments are mis-ordered
Current code produces-c:v -vf format=yuv420p h264_videotoolbox -q:v ..., which fails because-c:vlacks its codec argument. Correct order:case "videotoolbox": // macOS
- args.push(
- "-c:v",
- "-vf",
- "format=yuv420p",
- "h264_videotoolbox",
- "-q:v",
- vQuality
- );
- args.push(
- "-c:v",
- "h264_videotoolbox",
- "-vf",
- "format=yuv420p",
- "-q:v",
- vQuality
- );
Wrap AMF
constdeclarations in case blocks
Lines 290 and 307 declareconstdirectly in case clauses, violating Biome'snoSwitchDeclarations. Wrap each:case "hevc_amf": // AMD Windows HEVC
{
const amfHevcQuality = getAmfQuality(settings.speed);
args.push(...);
break;}
case "amf": // AMD Windows H264
{
const amfQuality = getAmfQuality(settings.speed);
args.push(...);
break;}
- Validate
videoQualitybeforeparseInt
Line 209 callsparseInt(settings.videoQuality).toString()without validation. IfvideoQualityis undefined, null, or non-numeric,parseIntreturnsNaN, passing the string"NaN"to ffmpeg. Add a guard (e.g.,parseInt(settings.videoQuality || 0)) or validate upstream.
🧹 Nitpick comments (2)
src/compressor.js (2)
401-429: Menu open/close behavior mirrors existing patterns; consider deduping fade logicThe
toggleMenu/closeMenupair gives a clear open/close contract and incorporates a fade‑out interval. This is consistent with the snippet insrc/common.js(and similar tosrc/playlist_new.js), but it does mean the fade logic is now duplicated across files.If you expect menu behavior to stay in sync across multiple windows, consider extracting the fade/close behavior into a shared helper (e.g., in
common.js) used by all windows instead of re‑implementing it here.
88-92: Theme toggle element exists, but null-checks remain recommended for defensive codingVerified that
compressor.htmlincludes thethemeToggleelement (lines 27–28:<select id="themeToggle" ...>), andcompressor.jsis only loaded by that page. Both lines 88–92 and 441–447 callgetId("themeToggle")without null-checks:
- Line 88:
getId("themeToggle").addEventListener("change", ...)- Line 445:
getId("themeToggle").value = storageThemeWhile the element presently exists, the
getId()function returns null for missing elements, and these accesses would throw. For robustness and maintainability, add null-checks before.addEventListener()and.valueassignments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/compressor.js(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/compressor.js (3)
translations/i18n-init.js (1)
i18n(2-2)src/playlist_new.js (4)
i18n(11-11)count(161-161)opacity(162-162)fade(163-171)src/common.js (3)
count(15-15)opacity(16-16)fade(17-26)
🪛 Biome (2.1.2)
src/compressor.js
[error] 290-290: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 307-307: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (6)
src/compressor.js (6)
28-39: Menu/navigation and compression button wiring look correctThe new event wiring for menu navigation and the compression/cancel buttons is straightforward and consistent: handlers are pure functions, and using helpers like
navigateTo/navigateToWinkeeps IPC calls centralized. As long as these elements exist on this page, this wiring is solid.Also applies to: 67-69
70-86: Output folder UX wiring is reasonableThe
output-folder-inputcheckbox toggles visibility of the custom folder button and clears the displayed path when unchecked, and the"directory-path"IPC handler cleanly updates and reveals the chosen path. This matches expected UX and keeps DOM updates localized.Also applies to: 94-98
185-203: Centralized success/error UI handlers look good
handleCompressionSuccessandhandleCompressionErrorpair nicely with the generalizedupdateProgress():
- Status classes are updated once via
updateProgress.- Clickable “file saved” and “error details” affordances are created in a focused place.
currentItemIdis reset in both paths.This is a solid refactor; just ensure the cancellation path is reconciled as noted earlier so errors aren’t shown for expected cancels.
Also applies to: 388-399
364-373: DOM helpers for IDs and selected file display are fine
getIdas a tiny wrapper plusupdateSelectedFiles()usingformatBytesto render name + size into#selected-filesis straightforward and readable. No functional concerns here.
431-439: Navigation helpers keep IPC wiring tidy
navigateToandnavigateToWinboth close the menu before sending IPC messages, which keeps the UI consistent across page transitions and centralizes the IPC channel names. Looks good.
358-362: AMF quality values verified as correctThe function correctly maps to valid AMD AMF quality parameters. The AMF encoders accept -quality with three values: speed, balanced, and quality, and
getAmfQualityreturns exactly these values. No issues.
| const VAAPI_DEVICE = "/dev/dri/renderD128"; | ||
| const FFMPEG_PATH = `"${getFfmpegPath()}"`; | ||
|
|
||
| console.log(`FFmpeg Path: ${FFMPEG_PATH}`); | ||
| /** @type {File[]} */ |
There was a problem hiding this comment.
🧩 Analysis chain
Fix i18n global access and consider handling missing FFmpeg path more defensively
- The
translations-loadedlistener useswindow.i18n.translatePage(), but other code in this file uses barei18n(e.g., Line 188, Line 198), and the provided snippet intranslations/i18n-init.jsshowsconst i18n = new I18n();. With aconstdeclaration,i18nis not attached towindow, sowindow.i18nmay beundefinedat runtime, causing a crash when this event fires. Align this to usei18n.translatePage()or explicitly attachi18ntowindowin the initializer so usage is consistent. FFMPEG_PATHis computed once at module load time and logged, which is fine. However, on platforms wheregetFfmpegPath()can legitimately return""(e.g., FreeBSD without ffmpeg in PATH), the app will still attempt to exec an empty command string later. You may want to short‑circuit with a user‑visible error ifgetFfmpegPath()returns a falsy value instead of proceeding toexecan invalid command.
Also applies to: 19-27
🌐 Web query:
In a standard browser/renderer environment (non-module scripts), does `const i18n = new I18n();` attach `i18n` to `window`, or is it only available as a global binding (i.e., `i18n` vs `window.i18n`)?
💡 Result:
Short answer: const i18n = new I18n(); (in a non-module, top-level script) creates a global lexical binding named i18n but does NOT create a property on window. So i18n works as an identifier, but window.i18n is undefined. [1][2]
Sources:
- MDN: Global object — "var statements and function declarations ... create properties of the global object. On the other hand, let and const declarations never create properties of the global object." [1]
- MDN: let (and const) — "let declarations do not create properties on globalThis when declared at the top level of a script." [2]
---
🏁 Script executed:
```shell
#!/bin/bash
# Find i18n declaration and window.i18n usage in src/compressor.js
echo "=== i18n declaration in translations/i18n-init.js ==="
fd -e js -p translations | head -5 | xargs grep -n "const i18n\|let i18n\|var i18n" 2>/dev/null | head -10
echo ""
echo "=== window.i18n usage in src/compressor.js ==="
rg -n "window\.i18n" src/compressor.js
echo ""
echo "=== bare i18n usage in src/compressor.js ==="
rg -n "i18n\." src/compressor.js | head -15
echo ""
echo "=== getFfmpegPath function ==="
rg -n "getFfmpegPath" src/compressor.js -A5 -B2
Length of output: 888
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full getFfmpegPath function implementation
echo "=== Full getFfmpegPath function ==="
rg -n "^function getFfmpegPath" src/compressor.js -A30
Length of output: 749
---
🏁 Script executed:
```shell
#!/bin/bash
# Check where FFMPEG_PATH is used and if there are guards
echo "=== FFMPEG_PATH usage in exec/spawn commands ==="
rg -n "FFMPEG_PATH" src/compressor.js -B2 -A2
echo ""
echo "=== exec/execSync/spawn patterns ==="
rg -n "exec\(|spawn\(" src/compressor.js -A3 -B2 | head -40
Length of output: 855
Fix i18n global access and add FFmpeg path validation
- Line 25 calls
window.i18n.translatePage(), buti18nis declared withconstattranslations/i18n-init.js:2. Per MDN,constdeclarations do not create properties onwindow, sowindow.i18nisundefinedand will crash at runtime when thetranslations-loadedevent fires. Use barei18n.translatePage()instead, or attachi18ntowindowexplicitly in the initializer. getFfmpegPath()returns an empty string on FreeBSD when ffmpeg is not found (line 538). This results inFFMPEG_PATHbecoming""(line 9), which produces an invalid command string passed toexec()at line 145. Add a guard ingetFfmpegPath()to throw or log an error if the path is empty, or validateFFMPEG_PATHbefore constructing the command at line 338.
🤖 Prompt for AI Agents
In src/compressor.js (referenced lines ~8-12, and also addressing calls at ~25,
145, 338 and getFfmpegPath at ~538), remove reliance on window.i18n (use
i18n.translatePage() directly or ensure i18n is attached to window in
translations initializer) so the translations-loaded handler doesn't crash, and
add validation for FFmpeg path: update getFfmpegPath() to throw or return a
clear error/log when it finds no binary (empty string) and/or validate
FFMPEG_PATH before building the exec command (abort with a logged error or throw
if empty) to prevent constructing an invalid command.
| function detectHardware() { | ||
| si.graphics().then((info) => { | ||
| console.log({gpuInfo: info}); | ||
| const gpuDevices = info.controllers; | ||
| const platform = os.platform(); | ||
|
|
||
| gpuDevices.forEach((gpu) => { | ||
| const gpuName = gpu.vendor.toLowerCase(); | ||
| const gpuModel = gpu.model.toLowerCase(); | ||
| const isNvidia = | ||
| gpuName.includes("nvidia") || gpuModel.includes("nvidia"); | ||
| const isAMD = | ||
| gpuName.includes("advanced micro devices") || | ||
| gpuModel.includes("amd"); | ||
| const isIntel = gpuName.includes("intel"); | ||
|
|
||
| if (isNvidia) { | ||
| showOptions(".nvidia_opt"); | ||
| } else if (isAMD) { | ||
| if (platform === "win32") showOptions(".amf_opt"); | ||
| else showOptions(".vaapi_opt"); | ||
| } else if (isIntel) { | ||
| if (platform === "win32") showOptions(".qsv_opt"); | ||
| else if (platform !== "darwin") showOptions(".vaapi_opt"); | ||
| } else { | ||
| if (platform === "darwin") showOptions(".videotoolbox_opt"); | ||
| } | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| function showOptions(selector) { | ||
| document.querySelectorAll(selector).forEach((opt) => { | ||
| opt.style.display = "block"; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Hardware detection logic may skip Videotoolbox on Intel macOS
The detectHardware() logic is generally sensible—using systeminformation.graphics() and os.platform() to show encoder‑specific options via CSS selectors—but there is a corner case:
- For Intel macOS,
gpu.vendor/gpu.modelwill likely matchisIntel, andplatform === "darwin". In theisIntelbranch, you only callshowOptions(".qsv_opt")forwin32and.vaapi_optfor non‑darwin platforms. The macOS path for Intel is effectively a no‑op. - The final
else { if (platform === "darwin") showOptions(".videotoolbox_opt"); }is only reached when the GPU is not recognized as Nvidia/AMD/Intel. So Intel macOS machines never reach the Videotoolbox branch.
If Videotoolbox is intended to be available on macOS regardless of GPU vendor, you may want a separate if (platform === "darwin") showOptions(".videotoolbox_opt") that runs independently of the vendor checks (or adjust the Intel branch accordingly).
Also, if multiple controllers are present, you may end up calling showOptions multiple times for the same selector, which is harmless but redundant.
🌐 Web query:
On macOS with Intel GPUs, is `ffmpeg`'s `h264_videotoolbox` usable regardless of GPU vendor, and should the UI always expose Videotoolbox options when `os.platform() === "darwin"`?
💡 Result:
Short answer: No — you can't blindly expose Videotoolbox on all macOS systems just because os.platform() === "darwin". VideoToolbox is the macOS hardware-acceleration API and, when available, FFmpeg's h264_videotoolbox will use whatever macOS-provided encoder exists (Intel QSV on some Intel Macs, AMD VCE on others, or Apple Media Engine on Apple Silicon). But availability and capabilities depend on the mac model, macOS version and how FFmpeg was built — so the UI should probe capability at runtime rather than assume presence by OS alone. [1][2][3]
Recommended implementation:
- Check that the running ffmpeg build advertises videotoolbox support (e.g. ffmpeg -hwaccels and ffmpeg -encoders / grep videotoolbox) and/or attempt a small probe encode. [4]
- Only show Videotoolbox options when those runtime checks succeed; optionally hide or disable them with an explanatory tooltip otherwise. [1][2]
Sources:
- HandBrake docs — VideoToolbox: VideoToolbox is the macOS hardware encoder, underlying vendor chosen by system; support varies by Mac model/OS. [1]
- Jellyfin docs — VideoToolbox used on macOS; support varies by Mac generation and may fall back to software for unsupported cases. [2]
- FFmpeg mailing / notes — h264_videotoolbox uses the VideoToolbox API and can be subject to platform/driver/resolution limits. [3]
- Practical FFmpeg guidance — how to detect hw accel and videocoders in ffmpeg builds. [4]
Let me verify if the codebase has any runtime capability checks or FFmpeg probing logic:
Now let me search the codebase to see if there's any existing FFmpeg capability detection:
Based on the web search verification, I can now provide the final rewritten review comment. The original concern is valid and confirmed by authoritative sources—Videotoolbox availability on macOS cannot be determined by OS check alone and requires runtime FFmpeg capability probing.
Fix hardware detection to probe FFmpeg Videotoolbox support at runtime, not by OS/GPU vendor alone.
The detectHardware() logic relies on os.platform() and GPU vendor checks to conditionally show encoder options, but this approach is architecturally insufficient. VideoToolbox is conditional on FFmpeg being built with the --enable-videotoolbox flag, and availability varies by Mac model, macOS version, and FFmpeg build configuration—not by OS alone.
Current issues:
- Intel macOS systems never reach the Videotoolbox branch (they only match the
isIntelcondition, which lacks a macOS path). - Even on macOS, blindly showing
.videotoolbox_optfor unknown GPU vendors is unreliable.
Recommended fix:
Use runtime checks like ffmpeg.getAvailableFormats() to probe whether the FFmpeg build supports h264_videotoolbox before showing Videotoolbox options. Only display the option when runtime capability detection succeeds.
🤖 Prompt for AI Agents
In src/compressor.js around lines 449-484, the current detectHardware() shows
Videotoolbox based only on OS/GPU vendor which is unreliable; update it to probe
FFmpeg at runtime for Videotoolbox support (e.g., call FFmpeg to list
encoders/formats or use your ffmpeg wrapper to getAvailableEncoders/formats) and
only call showOptions('.videotoolbox_opt') when the probe confirms
h264_videotoolbox (or the specific encoder/format) is available; also ensure the
Intel macOS path considers this runtime check (so Intel on macOS can show
Videotoolbox if FFmpeg supports it) and handle the async probe with proper
promise/async flow and error handling so UI toggles happen after the capability
check completes.
| function generateOutputPath(file, settings) { | ||
| console.log({settings}); | ||
| const output_extension = settings.extension; | ||
| const parsed_file = path.parse(file.path); | ||
| let outputDir = settings.outputPath || parsed_file.dir; | ||
|
|
||
| if (output_extension === "unchanged") { | ||
| return path.join( | ||
| outputDir, | ||
| `${parsed_file.name}${settings.outputSuffix}${parsed_file.ext}` | ||
| ); | ||
| } | ||
| return path.join( | ||
| outputDir, | ||
| `${parsed_file.name}_compressed.${output_extension}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Output path generation: suffix only applied for “unchanged” extension and debug log
generateOutputPath is a nice encapsulation of the previous path logic, but two details to consider:
settings.outputSuffixis only honored in the"unchanged"extension branch. If a user selects a different extension, the filename is always${name}_compressed.${extension}, and any configured suffix is ignored. If the UI intent is “append this suffix regardless of extension choice,” you’ll want to incorporatesettings.outputSuffixinto both branches.- The
console.log({settings});is likely a leftover debug statement; it can get noisy if many files are processed. Consider removing it or gating it behind a debug flag.
Example adjustment:
- if (output_extension === "unchanged") {
- return path.join(
- outputDir,
- `${parsed_file.name}${settings.outputSuffix}${parsed_file.ext}`
- );
- }
- return path.join(
- outputDir,
- `${parsed_file.name}_compressed.${output_extension}`
- );
+ const suffix = settings.outputSuffix || "";
+ if (output_extension === "unchanged") {
+ return path.join(
+ outputDir,
+ `${parsed_file.name}${suffix}${parsed_file.ext}`
+ );
+ }
+ return path.join(
+ outputDir,
+ `${parsed_file.name}${suffix}_compressed.${output_extension}`
+ );🤖 Prompt for AI Agents
In src/compressor.js around lines 503-519, remove the stray debug console.log
and update filename construction to honor settings.outputSuffix in both
branches: ensure outputSuffix defaults to an empty string if falsy, keep the
unchanged branch building `${name}${outputSuffix}${ext}` and change the other
branch to include the suffix before the `_compressed` token (e.g.
`${name}${outputSuffix}_compressed.${output_extension}`), preserving the
existing outputPath fallback logic; also optionally normalize/trim outputSuffix
to avoid accidental separators.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Refactor