Add PySide6 GUI with video browser and full settings - #44
Merged
Conversation
Complete desktop GUI for UltraSinger with: - **YouTube Browser**: Embedded QWebEngineView with persistent session, navigation toolbar, cookie status indicator, and floating "Convert" button overlay that captures the current URL - **Cookie Management**: Automatic extraction from QWebEngineCookieStore, Netscape-format export for yt-dlp, persistent browser profile - **Conversion Settings**: All CLI parameters exposed as form controls, grouped in card-based sections (Whisper, Language, Post-Processing, LLM Correction, Output, Device, Paths) - **Queue & Progress**: Real-time log output with color-coded lines, stage detection, elapsed timer, cancel support, output folder opener - **Preferences**: Default output folder, Groq/LLM API configuration with setup guide, cookie import/export/clear - **Custom Widgets**: Toggle switches with animation, sidebar navigation, settings cards, drag-and-drop file zone, color-coded log viewer - **Dark Theme**: Comprehensive QSS stylesheet with modern dark aesthetic, pink accent color, custom-styled scrollbars/inputs/buttons - **Docker Compose**: GPU-enabled docker-compose.yml for CLI usage Install: `uv sync --extra gui` Launch: `cd src && uv run python gui_main.py` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a complete PySide6-based graphical user interface for UltraSinger, including a YouTube browser tab with integrated conversion controls, form-based settings and preferences panels, a conversion queue with real-time logging and progress tracking, and secure credential management via system keyring integration. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as BrowserTab
participant MainWindow
participant Settings as SettingsTab
participant Queue as QueueTab
participant Runner as UltraSingerRunner
participant CLI as UltraSinger CLI
User->>Browser: Click YouTube video, press Convert
Browser->>MainWindow: convert_requested(url)
MainWindow->>MainWindow: Set video input in sidebar
User->>Settings: Configure transcription/language options
User->>MainWindow: Click "Start Conversion" button
MainWindow->>Settings: collect_config()
MainWindow->>Queue: start_conversion(args, output_folder)
Queue->>Runner: start(args)
Runner->>CLI: subprocess.Popen(uv run ...)
CLI-->>Runner: stdout/stderr lines
Runner->>Queue: line_output signal
Queue->>Queue: append_log(line), detect_stage()
CLI-->>Runner: finished(exit_code)
Runner->>Queue: finished(exit_code)
Queue->>Queue: Update status, enable Open Output
Queue-->>User: Display conversion result & logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
src/gui/widgets/animated_button.py (1)
1-15: Class name and docstring are misleading.The class is named
AnimatedButtonand the docstring mentions "hover animation effects," but there's no animation logic implemented. The actual behavior is setting styling properties (cursor, object name, height). If CSS-based hover transitions are intended viastyles.qss, consider renaming toStyledButtonorPrimaryButton, or clarifying in the docstring that animations come from the stylesheet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/animated_button.py` around lines 1 - 15, The class AnimatedButton and its docstring claim hover animation but contain no animation logic; update either the name or the docstring to reflect the actual behavior: either rename the class (e.g., StyledButton or PrimaryButton) everywhere it’s used or change the class docstring to state that styling/hover effects are provided by the app stylesheet (styles.qss) rather than by internal animation code; adjust references to AnimatedButton in code and tests if renaming, or update the __init__ docstring and any comments around setObjectName("primaryButton") and setCursor to make it clear animations are CSS-driven.src/gui/widgets/log_viewer.py (1)
5-5: Unused import:QTimer.
QTimeris imported but not used anywhere in this module.🧹 Remove unused import
-from PySide6.QtCore import Qt, QTimer +from PySide6.QtCore import Qt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/log_viewer.py` at line 5, The import line brings QTimer into scope but it is unused in this module; remove QTimer from the from PySide6.QtCore import ... statement (leaving Qt) to clean up the unused import in log_viewer.py (remove the QTimer token from the import list).src/gui/widgets/sidebar.py (1)
69-69: Hardcoded version string.The version
"v0.0.13.dev16"is duplicated here and inpyproject.toml. Consider reading from a single source (e.g.,importlib.metadata.version("UltraSinger")) to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/sidebar.py` at line 69, Replace the hardcoded version string used to construct version_label in sidebar.py with a single-source lookup — call importlib.metadata.version("UltraSinger") (or importlib_metadata.version for older Python) to fetch the package version and use that value when creating the QLabel (e.g., version_label = QLabel(f"v{version}")). Add a small try/except fallback to a sensible default like "v0.0.0" if the import or lookup fails so the GUI still loads. Ensure you update the symbol version_label and any nearby code that formats or displays the version to use the fetched value instead of the literal string.src/gui/settings_tab.py (1)
473-476: Avoid mutatingSettingsCard's private layout from the tab.Line 475 is reaching into another widget's private state and assuming the last item is the row you just added. That is brittle, and the same workaround already exists in
PreferencesTab. A small public helper onSettingsCardfor labeled custom rows would make these compound rows safe to reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/settings_tab.py` around lines 473 - 476, The tab mutates SettingsCard's private _layout (card._layout.removeItem(...)) after calling card.add_row; instead add a public helper on SettingsCard (e.g., add_labeled_row or add_custom_row) that accepts a label string and a QWidget/layout so callers like this settings_tab can create the labeled MuseScore row by calling that helper instead of poking card._layout; update the tab to call the new SettingsCard method (mirroring the pattern used in PreferencesTab) and remove the direct access to card._layout and the manual removeItem call.src/gui/main_window.py (2)
95-98: Avoid reaching intoQueueTabprivate internals fromMainWindow.At Line 95,
self._queue_tab._log.append_line(...)couples this class to a private implementation detail. Prefer a publicQueueTabmethod (for example,append_error(...)) and call that instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/main_window.py` around lines 95 - 98, MainWindow is directly accessing QueueTab's private _log by calling self._queue_tab._log.append_line(...); introduce a public method on QueueTab (e.g., append_error or append_line) that accepts the error text and handles logging, then replace the call in MainWindow to use self._queue_tab.append_error("No input source selected. Select a YouTube URL or local file first.") so MainWindow stops depending on _log and uses the public API of QueueTab.
126-133: Narrow close-time exception handling.At Line 132, catching
Exceptionis too broad and can mask non-recoverable bugs in settings collection. Catch expected persistence-related exceptions only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/main_window.py` around lines 126 - 133, The try/except around calling _settings_tab.collect_config, _preferences_tab.collect_preferences and save_config is too broad; replace the bare "except Exception" with handling only expected persistence/parsing errors (e.g., IOError/OSError, json.JSONDecodeError, ValueError or your app's persistence error) and log those with logger.warning(..., exc_info=True), but allow other unexpected exceptions to propagate (or re-raise after logging) so real bugs aren't masked; adjust the block around collect_config, collect_preferences and save_config accordingly.src/gui/config.py (1)
71-76: Avoid blindexcept Exceptionin config IO paths.At Line 75 and Line 86, broad catches reduce signal for real defects. Catch expected exception classes (
json.JSONDecodeError,TypeError,OSError) instead.Also applies to: 83-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/config.py` around lines 71 - 76, The try/except around opening and loading _CONFIG_FILE uses a broad except; change it to catch expected errors only (e.g., json.JSONDecodeError, TypeError, OSError) so real bugs surface: in the block that opens _CONFIG_FILE, catch those specific exceptions and call logger.warning(..., exc_info=True) as before, and do the same for the later load/save block that uses config.update and file operations; reference the symbols _CONFIG_FILE, json.load, config.update, and logger.warning when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 10-16: The usage examples are resolving against WORKDIR
/app/UltraSinger/src while volumes are mounted at
/app/UltraSinger/{input,output,cookies.txt}; update the example commands that
use relative paths (e.g. -i input/song.mp3, -o output/, --cookiefile
cookies.txt) to use the absolute mount paths (/app/UltraSinger/input/song.mp3,
/app/UltraSinger/output/, /app/UltraSinger/cookies.txt), or alternatively change
the example to run from the repo root by prefixing the command with cd
/app/UltraSinger && ... , or adjust the docker-compose volume mounts to
/app/UltraSinger/src/{input,output,cookies.txt} so the relative paths match the
WORKDIR.
In `@src/gui/browser_tab.py`:
- Around line 143-147: After connecting the cookie signal, call the existing
_update_cookie_status() immediately so the cookie indicator is initialized at
startup; specifically, after
self.cookie_manager.cookies_changed.connect(self._update_cookie_status) invoke
self._update_cookie_status() before loading the URL (the current call to
self._view.setUrl(QUrl("https://www.youtube.com"))).
In `@src/gui/config.py`:
- Line 39: The config currently stores "llm_api_key" and writes the config file
directly (around the write block that writes to the target path), which risks
corruption and exposes secrets; change the save logic to perform an atomic
replace and lock down permissions: write the JSON to a temporary file in the
same directory (e.g., using tempfile.NamedTemporaryFile(delete=False,
dir=os.path.dirname(target_path))), fsync the temp file, os.fchmod or os.chmod
the temp file to 0o600 (owner read/write only) to restrict permissions, then
atomically replace the original with os.replace(temp_path, target_path); also
ensure any temp file is unlinked on error. Locate the code by searching for the
"llm_api_key" key and the block that writes to the target path (the write block
around lines ~80-87) and update that function to use the temp-write + chmod +
os.replace pattern.
In `@src/gui/main_window.py`:
- Around line 86-89: The cookie export calls to
self._browser_tab.cookie_manager.export_netscape(cookie_path) can raise on bad
paths or write failures and must be hardened: wrap each export_netscape
invocation (the ones using cookie_path from self._config and the later export
call guarded by has_youtube_cookies) in a try/except, validate cookie_path is
non-empty (and optionally expand user paths), catch any exceptions, and log them
with logger.error (including the exception info) while returning/continuing so
the navigation/conversion flow does not crash; ensure you reference and update
the code around cookie_path, self._browser_tab.cookie_manager.export_netscape,
has_youtube_cookies, and logger.info/logger.error.
In `@src/gui/preferences_tab.py`:
- Around line 191-199: The _save() method is failing to persist the cookie_file
returned by collect_preferences(): call collect_preferences() (or otherwise
retrieve the cookie_file value returned by collect_preferences()) and assign it
into self._config under the same key used elsewhere (e.g. "cookie_file") before
calling save_config(self._config); update the _save() in preferences_tab (and
the analogous save block around collect_preferences at lines ~201-209) so all
returned values from collect_preferences() are written back into self._config
prior to persisting.
In `@src/gui/settings_tab.py`:
- Around line 438-453: _bpm_override and _octave_shift are free-text QLineEdit
fields that cause type conversion errors downstream (UltraSinger.py expects
--bpm as float and --octave as int); replace them with numeric widgets (use
QDoubleSpinBox for _bpm_override and QSpinBox for _octave_shift) or attach
QDoubleValidator/QIntValidator, set sensible ranges/step/nullable behavior
(e.g., allow empty/None by using a checkbox or special sentinel), and update
collect_config() to return a float for bpm and an int for octave (or None when
empty). Also apply the same change to the equivalent fields referenced later
(the other occurrence mentioned) so command-line args built for --bpm and
--octave are always valid numeric types.
In `@src/gui/ultrasinger_runner.py`:
- Around line 50-52: The command string is being logged with secrets included;
before calling self.line_output.emit in ultrasinger_runner.py (where cmd is
built from _build_command(project_root) + self._args and then joined), create a
redacted copy that masks values for secret flags (e.g., flags named
--llm_api_key, --api_key, --secret, -k, etc.) by replacing the next argv item or
the --flag=value portion with a fixed token like "<REDACTED>", then log the
joined redacted list instead of the raw cmd; update the code paths that build or
log cmd (references: _build_command, self._args, and the self.line_output.emit
call) to use this redacted copy so secrets are never printed.
- Around line 47-56: The run() method must check the cancellation flag before
spawning the subprocess and move any blocking wait/kill logic into a background
thread so the GUI thread is never blocked; specifically, at the start of run()
(after building cmd) return early if self._cancelled is True to avoid spawning
_process, spawn subprocess.Popen into self._process only when not cancelled, and
then start a separate daemon thread (e.g., _monitor_process or similar) to
perform the blocking wait(timeout=5) and subsequent kill() if needed; adjust
cancel() to set self._cancelled = True and, if self._process is already set,
call self._process.kill()/terminate() and let the monitor thread handle the
wait/cleanup so cancel() itself does not perform any blocking operations.
In `@src/gui/widgets/file_drop_zone.py`:
- Around line 19-25: The FileDropZone widget is not keyboard accessible; make it
focusable and respond to keyboard activation by setting a focus policy (e.g.,
Qt.StrongFocus) and an accessible name/role, and implement/override
keyPressEvent to trigger the same file-picker action as mouse activation (reuse
the existing mousePressEvent or the file-open method used by FileDropZone). Also
ensure the widget exposes a clear accessibleName (e.g., "Local file picker") so
screen readers can announce it and that setProperty("dragOver", False) remains
unchanged.
In `@src/gui/widgets/settings_card.py`:
- Around line 55-58: The tooltip is only applied to the label in the toggle row,
causing inconsistent behavior compared to add_row; update the toggle row code in
settings_card.py so that when tooltip is present you call setToolTip(tooltip) on
the toggle widget as well as on label (i.e., ensure both
label.setToolTip(tooltip) and toggle.setToolTip(tooltip) are invoked for the
toggle row creation function/method).
In `@src/gui/widgets/sidebar.py`:
- Line 59: The lambda connected to btn.clicked in add_section captures the loop
variable index by reference so every button emits the final index; change the
connection to capture the current index as a default argument and accept the
clicked bool (e.g., use a lambda like lambda checked=False, idx=index:
self.section_changed.emit(idx) or use functools.partial to bind idx) so each
button emits its own index when clicked; update the btn.clicked.connect call and
ensure self.section_changed.emit receives the bound idx.
---
Nitpick comments:
In `@src/gui/config.py`:
- Around line 71-76: The try/except around opening and loading _CONFIG_FILE uses
a broad except; change it to catch expected errors only (e.g.,
json.JSONDecodeError, TypeError, OSError) so real bugs surface: in the block
that opens _CONFIG_FILE, catch those specific exceptions and call
logger.warning(..., exc_info=True) as before, and do the same for the later
load/save block that uses config.update and file operations; reference the
symbols _CONFIG_FILE, json.load, config.update, and logger.warning when making
the change.
In `@src/gui/main_window.py`:
- Around line 95-98: MainWindow is directly accessing QueueTab's private _log by
calling self._queue_tab._log.append_line(...); introduce a public method on
QueueTab (e.g., append_error or append_line) that accepts the error text and
handles logging, then replace the call in MainWindow to use
self._queue_tab.append_error("No input source selected. Select a YouTube URL or
local file first.") so MainWindow stops depending on _log and uses the public
API of QueueTab.
- Around line 126-133: The try/except around calling
_settings_tab.collect_config, _preferences_tab.collect_preferences and
save_config is too broad; replace the bare "except Exception" with handling only
expected persistence/parsing errors (e.g., IOError/OSError,
json.JSONDecodeError, ValueError or your app's persistence error) and log those
with logger.warning(..., exc_info=True), but allow other unexpected exceptions
to propagate (or re-raise after logging) so real bugs aren't masked; adjust the
block around collect_config, collect_preferences and save_config accordingly.
In `@src/gui/settings_tab.py`:
- Around line 473-476: The tab mutates SettingsCard's private _layout
(card._layout.removeItem(...)) after calling card.add_row; instead add a public
helper on SettingsCard (e.g., add_labeled_row or add_custom_row) that accepts a
label string and a QWidget/layout so callers like this settings_tab can create
the labeled MuseScore row by calling that helper instead of poking card._layout;
update the tab to call the new SettingsCard method (mirroring the pattern used
in PreferencesTab) and remove the direct access to card._layout and the manual
removeItem call.
In `@src/gui/widgets/animated_button.py`:
- Around line 1-15: The class AnimatedButton and its docstring claim hover
animation but contain no animation logic; update either the name or the
docstring to reflect the actual behavior: either rename the class (e.g.,
StyledButton or PrimaryButton) everywhere it’s used or change the class
docstring to state that styling/hover effects are provided by the app stylesheet
(styles.qss) rather than by internal animation code; adjust references to
AnimatedButton in code and tests if renaming, or update the __init__ docstring
and any comments around setObjectName("primaryButton") and setCursor to make it
clear animations are CSS-driven.
In `@src/gui/widgets/log_viewer.py`:
- Line 5: The import line brings QTimer into scope but it is unused in this
module; remove QTimer from the from PySide6.QtCore import ... statement (leaving
Qt) to clean up the unused import in log_viewer.py (remove the QTimer token from
the import list).
In `@src/gui/widgets/sidebar.py`:
- Line 69: Replace the hardcoded version string used to construct version_label
in sidebar.py with a single-source lookup — call
importlib.metadata.version("UltraSinger") (or importlib_metadata.version for
older Python) to fetch the package version and use that value when creating the
QLabel (e.g., version_label = QLabel(f"v{version}")). Add a small try/except
fallback to a sensible default like "v0.0.0" if the import or lookup fails so
the GUI still loads. Ensure you update the symbol version_label and any nearby
code that formats or displays the version to use the fetched value instead of
the literal string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac4625da-1cb3-4028-b4ae-234938d784d5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
docker-compose.ymlpyproject.tomlsrc/gui/__init__.pysrc/gui/browser_tab.pysrc/gui/config.pysrc/gui/cookie_manager.pysrc/gui/main_window.pysrc/gui/preferences_tab.pysrc/gui/queue_tab.pysrc/gui/resources/styles.qsssrc/gui/settings_tab.pysrc/gui/ultrasinger_runner.pysrc/gui/widgets/__init__.pysrc/gui/widgets/animated_button.pysrc/gui/widgets/file_drop_zone.pysrc/gui/widgets/log_viewer.pysrc/gui/widgets/settings_card.pysrc/gui/widgets/sidebar.pysrc/gui/widgets/toggle_switch.pysrc/gui_main.py
Actionable fixes: - docker-compose.yml: Use absolute paths in usage examples to match WORKDIR - browser_tab.py: Call _update_cookie_status() before loading URL - config.py: Atomic write + chmod 0o600 for API key security; narrow exception handling to json.JSONDecodeError, TypeError, OSError - main_window.py: Wrap cookie export calls in try/except OSError; narrow closeEvent except to specific exceptions - preferences_tab.py: _save() now persists cookie_file via collect_preferences() - settings_tab.py: Add QDoubleValidator for BPM, QIntValidator for octave - ultrasinger_runner.py: Redact --llm_api_key from logged command; check _cancelled before spawning subprocess - file_drop_zone.py: Add keyboard accessibility (focus policy + keyPressEvent) - settings_card.py: Apply tooltip to toggle widget in add_toggle_row - sidebar.py: Fix lambda loop variable capture with default argument Nitpick fixes: - animated_button.py: Fix module/class docstrings to match actual behavior - log_viewer.py: Remove unused QTimer import - sidebar.py: Use importlib.metadata.version() instead of hardcoded string - settings_card.py: Add remove_last_item() public method; replace _layout direct access in settings_tab.py and preferences_tab.py - queue_tab.py: Add append_log() public method; use it from main_window.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/gui/ultrasinger_runner.py (1)
97-105:⚠️ Potential issue | 🟠 Major
cancel()still blocks the UI path for up to 5 seconds.
QueueTab._on_cancel()triggersUltraSingerRunner.cancel(), which directly calls workercancel(); Line 103 then blocks withwait(timeout=5). This can freeze UI responsiveness during cancel.🛠️ Non-blocking cancellation pattern
import logging import shutil import subprocess import sys +import threading from pathlib import Path @@ def cancel(self): """Terminate the subprocess.""" self._cancelled = True if self._process and self._process.poll() is None: self._process.terminate() - try: - self._process.wait(timeout=5) - except subprocess.TimeoutExpired: - self._process.kill() + threading.Thread(target=self._wait_and_kill_if_needed, daemon=True).start() + + def _wait_and_kill_if_needed(self): + process = self._process + if not process: + return + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + if process.poll() is None: + process.kill()#!/bin/bash # Verify the synchronous blocking cancel path from UI -> runner -> worker. rg -n -C3 'def _on_cancel|def cancel|self\._worker\.cancel\(|wait\(timeout=5\)' src/gui/queue_tab.py src/gui/ultrasinger_runner.pyAlso applies to: 159-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/ultrasinger_runner.py` around lines 97 - 105, The cancel() implementation in UltraSingerRunner blocks the UI by calling self._process.wait(timeout=5) synchronously (triggered from QueueTab._on_cancel()); change cancel() to perform the terminate/wait/kill sequence off the UI thread—e.g., start a short-lived background thread or submit a task to the existing worker thread that calls self._process.terminate(), then waits up to 5s and calls kill() on TimeoutExpired—so cancel() returns immediately; apply the same non-blocking pattern for the duplicate block around the other wait usage (the second cancel-like sequence at lines ~159-163) and reference the _process, cancel(), and QueueTab._on_cancel symbols when updating the code.src/gui/widgets/file_drop_zone.py (1)
25-26:⚠️ Potential issue | 🟡 MinorFinalize keyboard/a11y behavior by naming the control and accepting handled key events.
This widget is focusable, but without an accessible name it may be announced generically. Also, handled Enter/Space should explicitly accept/return.
♿ Suggested follow-up
self.setFocusPolicy(Qt.FocusPolicy.StrongFocus) + self.setAccessibleName("Local file picker") self.setProperty("dragOver", False) @@ def keyPressEvent(self, event): """Open file dialog on Enter or Space key press.""" if event.key() in (Qt.Key.Key_Return, Qt.Key.Key_Enter, Qt.Key.Key_Space): self._open_file_dialog() + event.accept() + return else: super().keyPressEvent(event)Also applies to: 72-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/file_drop_zone.py` around lines 25 - 26, The widget is focusable but lacks an accessible name and does not mark handled key events as accepted; in the FileDropZone constructor call self.setAccessibleName("File drop zone") (or another clear label) alongside the existing self.setFocusPolicy and self.setProperty, and implement/modify keyPressEvent(self, event) in the FileDropZone class to check for Enter/Return/Space (Qt.Key_Return, Qt.Key_Enter, Qt.Key_Space), call the existing activation/submit handler (or perform the same action as mouse drop), then call event.accept() and return (do not fall through to super for handled keys); apply the same setAccessibleName and keyPressEvent acceptance changes to the other focusable instance(s) referenced around the 72-77 block.src/gui/widgets/settings_card.py (1)
69-73:⚠️ Potential issue | 🟡 MinorIncomplete cleanup when removing layout items.
removeItem()only removes the item from the layout's management but does not delete or hide any associated widget. The widget will remain visible and parented, potentially causing visual artifacts or memory accumulation ifremove_last_itemis called repeatedly.Proposed fix to properly clean up widgets and layouts
def remove_last_item(self): """Remove the last item from the card layout.""" - last = self._layout.itemAt(self._layout.count() - 1) - if last: - self._layout.removeItem(last) + if self._layout.count() == 0: + return + last = self._layout.takeAt(self._layout.count() - 1) + if last.widget(): + last.widget().deleteLater() + elif last.layout(): + # Recursively delete nested layout contents + while last.layout().count(): + child = last.layout().takeAt(0) + if child.widget(): + child.widget().deleteLater()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/settings_card.py` around lines 69 - 73, remove_last_item currently calls self._layout.removeItem(last) which only detaches the QLayoutItem but leaves any associated widget/layout visible and parented; change the logic to remove and fully clean up the item: retrieve the QLayoutItem (use self._layout.takeAt(self._layout.count()-1) or the existing last), then if last.widget() exists call widget.hide(); widget.setParent(None); widget.deleteLater(); if last.layout() exists recursively clear its children and call deleteLater() on child widgets/layouts; finally ensure the removed QLayoutItem is deleted or allowed to be GC'd. Use the symbols remove_last_item, self._layout, last, last.widget(), last.layout(), and widget.deleteLater() to locate and implement the fix.
🧹 Nitpick comments (3)
src/gui/main_window.py (1)
29-29: Avoid hard-coded app version in the window title.Using a fixed version string here will drift from packaged runtime version.
♻️ Suggested refactor
+import importlib.metadata @@ - self.setWindowTitle("UltraSinger v0.0.13.dev16") + try: + version = importlib.metadata.version("ultrasinger") + except importlib.metadata.PackageNotFoundError: + version = "dev" + self.setWindowTitle(f"UltraSinger v{version}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/main_window.py` at line 29, The window title currently hard-codes the app version in the call to self.setWindowTitle("UltraSinger v0.0.13.dev16"); change it to construct the title at runtime using the application's actual version (for example by reading QCoreApplication.applicationVersion() or an exported __version__/VERSION constant from the package metadata) and pass that formatted string to self.setWindowTitle; update the MainWindow initializer (or wherever self.setWindowTitle is called) to import or query the canonical version source and format "UltraSinger v{version}" instead of the fixed literal.src/gui/browser_tab.py (2)
111-121: Consider adding accessible names for navigation buttons.The navigation buttons use Unicode symbols (
◀,▶,↻) without accessible names. Screen reader users won't know what these buttons do. AddingsetAccessibleName()would improve accessibility.Proposed fix to add accessible names
self._back_btn = QPushButton("\u25C0") + self._back_btn.setAccessibleName("Back") self._back_btn.clicked.connect(self._view.back) tb_layout.addWidget(self._back_btn) self._fwd_btn = QPushButton("\u25B6") + self._fwd_btn.setAccessibleName("Forward") self._fwd_btn.clicked.connect(self._view.forward) tb_layout.addWidget(self._fwd_btn) self._refresh_btn = QPushButton("\u21BB") + self._refresh_btn.setAccessibleName("Refresh") self._refresh_btn.clicked.connect(self._view.reload)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/browser_tab.py` around lines 111 - 121, Add accessible names to the navigation QPushButton instances so screen readers can announce their purpose: call setAccessibleName(...) on the _back_btn, _fwd_btn, and _refresh_btn (e.g., "Back", "Forward", "Refresh") after each button is created in the BrowserTab GUI code where _back_btn, _fwd_btn, and _refresh_btn are instantiated and before adding them to tb_layout; ensure the accessible names are clear and match the connected actions (_view.back, _view.forward, _view.reload).
75-83: Hardcoded user agent version will become outdated.The Chrome version
131.0.0.0in the user agent string will age over time. Some websites may eventually flag older user agents. Consider either:
- Accepting this as a known limitation
- Deriving the version from
QWebEngineProfile.defaultUserAgent()and patching it- Making it configurable
This is a minor maintenance concern and acceptable for an initial implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/browser_tab.py` around lines 75 - 83, The hardcoded Chrome version in the user agent set via self._profile.setHttpUserAgent is brittle; update the code to generate the UA dynamically instead of the literal "Chrome/131.0.0.0". For example, retrieve the current default agent from QWebEngineProfile.defaultProfile().httpUserAgent() or QWebEngineProfile.defaultUserAgent(), parse/extract its Chrome/major.minor string and splice that into the custom UA built around get_browser_profile_path()/QWebEngineProfile, or make the UA string configurable (e.g., via a config value or constructor parameter) so setHttpUserAgent uses the derived or configured value rather than a hardcoded version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/config.py`:
- Around line 75-79: In load_config(), currently config.update(stored) assumes
the JSON payload is a dict; first validate that the loaded value (stored) is a
mapping before updating: after json.load(f) check isinstance(stored, dict) (or
collections.abc.Mapping) and only call config.update(stored) when true,
otherwise log a warning that the config file has unexpected JSON shape and skip
applying it (or replace with an empty dict) so startup recovery path is used;
reference the load_config() function and the stored/config variables to locate
where to add this validation and logging.
In `@src/gui/preferences_tab.py`:
- Around line 180-186: In _export_cookies, guard calls to
self._cookie_manager.export_netscape by first using the current UI path value
(read from self._cookie_path.text() if non-empty, falling back to
self._config.get("cookie_file", "")), wrap the export_netscape(...) call in a
try/except that catches OSError/IOError/Exception, log or display a non-fatal
error to the user (e.g. via a QMessageBox) on failure, and only call
self._cookie_path.setText(...) when export succeeds; ensure _cookie_manager is
still checked before attempting export to avoid breaking the Preferences flow.
In `@src/gui/queue_tab.py`:
- Around line 116-119: At conversion start reset the label styles so previous
colored styles don't persist: in the block where _status_icon, _status_label,
and _stage_label are updated (the conversion-start sequence using _status_icon,
_status_label, and _stage_label), call setStyleSheet("") on _status_label (and
_stage_label if needed) to clear any prior color/formatting before setting the
new text and icon.
In `@src/gui/ultrasinger_runner.py`:
- Around line 172-275: Missing mapping for the calculate_score GUI setting:
update build_args in ultrasinger_runner.py to append "--calculate_score" when
config.get("calculate_score") is truthy (add this check after the LLM correction
block inside build_args), and then add the corresponding long option and parser
handling in UltraSinger.py by adding "calculate_score" to the long-options list
and handling it in the argument parsing switch/case so the CLI recognizes and
sets the calculate_score behavior.
In `@src/gui/widgets/animated_button.py`:
- Around line 13-14: The primary flag currently sets the Qt object name via
setObjectName("primaryButton") in AnimatedButton (primary) but the app
stylesheet (styles.qss) has no corresponding `#primaryButton` rule, so change the
stylesheet or the widget naming: add a `#primaryButton` selector block to
src/gui/resources/styles.qss with the visual rules you want for primary buttons
(colors, border, hover, pressed states) so the setObjectName("primaryButton")
call takes effect, or alternatively change the AnimatedButton code to set an
object name that matches an existing rule in styles.qss; ensure the object name
string used in setObjectName(...) and the selector in styles.qss are identical.
---
Duplicate comments:
In `@src/gui/ultrasinger_runner.py`:
- Around line 97-105: The cancel() implementation in UltraSingerRunner blocks
the UI by calling self._process.wait(timeout=5) synchronously (triggered from
QueueTab._on_cancel()); change cancel() to perform the terminate/wait/kill
sequence off the UI thread—e.g., start a short-lived background thread or submit
a task to the existing worker thread that calls self._process.terminate(), then
waits up to 5s and calls kill() on TimeoutExpired—so cancel() returns
immediately; apply the same non-blocking pattern for the duplicate block around
the other wait usage (the second cancel-like sequence at lines ~159-163) and
reference the _process, cancel(), and QueueTab._on_cancel symbols when updating
the code.
In `@src/gui/widgets/file_drop_zone.py`:
- Around line 25-26: The widget is focusable but lacks an accessible name and
does not mark handled key events as accepted; in the FileDropZone constructor
call self.setAccessibleName("File drop zone") (or another clear label) alongside
the existing self.setFocusPolicy and self.setProperty, and implement/modify
keyPressEvent(self, event) in the FileDropZone class to check for
Enter/Return/Space (Qt.Key_Return, Qt.Key_Enter, Qt.Key_Space), call the
existing activation/submit handler (or perform the same action as mouse drop),
then call event.accept() and return (do not fall through to super for handled
keys); apply the same setAccessibleName and keyPressEvent acceptance changes to
the other focusable instance(s) referenced around the 72-77 block.
In `@src/gui/widgets/settings_card.py`:
- Around line 69-73: remove_last_item currently calls
self._layout.removeItem(last) which only detaches the QLayoutItem but leaves any
associated widget/layout visible and parented; change the logic to remove and
fully clean up the item: retrieve the QLayoutItem (use
self._layout.takeAt(self._layout.count()-1) or the existing last), then if
last.widget() exists call widget.hide(); widget.setParent(None);
widget.deleteLater(); if last.layout() exists recursively clear its children and
call deleteLater() on child widgets/layouts; finally ensure the removed
QLayoutItem is deleted or allowed to be GC'd. Use the symbols remove_last_item,
self._layout, last, last.widget(), last.layout(), and widget.deleteLater() to
locate and implement the fix.
---
Nitpick comments:
In `@src/gui/browser_tab.py`:
- Around line 111-121: Add accessible names to the navigation QPushButton
instances so screen readers can announce their purpose: call
setAccessibleName(...) on the _back_btn, _fwd_btn, and _refresh_btn (e.g.,
"Back", "Forward", "Refresh") after each button is created in the BrowserTab GUI
code where _back_btn, _fwd_btn, and _refresh_btn are instantiated and before
adding them to tb_layout; ensure the accessible names are clear and match the
connected actions (_view.back, _view.forward, _view.reload).
- Around line 75-83: The hardcoded Chrome version in the user agent set via
self._profile.setHttpUserAgent is brittle; update the code to generate the UA
dynamically instead of the literal "Chrome/131.0.0.0". For example, retrieve the
current default agent from QWebEngineProfile.defaultProfile().httpUserAgent() or
QWebEngineProfile.defaultUserAgent(), parse/extract its Chrome/major.minor
string and splice that into the custom UA built around
get_browser_profile_path()/QWebEngineProfile, or make the UA string configurable
(e.g., via a config value or constructor parameter) so setHttpUserAgent uses the
derived or configured value rather than a hardcoded version.
In `@src/gui/main_window.py`:
- Line 29: The window title currently hard-codes the app version in the call to
self.setWindowTitle("UltraSinger v0.0.13.dev16"); change it to construct the
title at runtime using the application's actual version (for example by reading
QCoreApplication.applicationVersion() or an exported __version__/VERSION
constant from the package metadata) and pass that formatted string to
self.setWindowTitle; update the MainWindow initializer (or wherever
self.setWindowTitle is called) to import or query the canonical version source
and format "UltraSinger v{version}" instead of the fixed literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49ac81c0-3ae3-4e1c-b612-9aae754e9fc9
📒 Files selected for processing (13)
docker-compose.ymlsrc/gui/browser_tab.pysrc/gui/config.pysrc/gui/main_window.pysrc/gui/preferences_tab.pysrc/gui/queue_tab.pysrc/gui/settings_tab.pysrc/gui/ultrasinger_runner.pysrc/gui/widgets/animated_button.pysrc/gui/widgets/file_drop_zone.pysrc/gui/widgets/log_viewer.pysrc/gui/widgets/settings_card.pysrc/gui/widgets/sidebar.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docker-compose.yml
- src/gui/widgets/log_viewer.py
Comment on lines
+172
to
+275
| def build_args(self, config: dict, input_source: str) -> list[str]: | ||
| """Build CLI argument list from configuration dictionary.""" | ||
| args = ["-i", input_source] | ||
|
|
||
| if config.get("output_folder"): | ||
| args.extend(["-o", config["output_folder"]]) | ||
|
|
||
| # Cookie file | ||
| if config.get("cookie_file") and Path(config["cookie_file"]).exists(): | ||
| args.extend(["--cookiefile", config["cookie_file"]]) | ||
|
|
||
| # Whisper settings | ||
| if config.get("whisper_model"): | ||
| args.extend(["--whisper", config["whisper_model"]]) | ||
| if config.get("whisper_batch_size"): | ||
| args.extend(["--whisper_batch_size", str(config["whisper_batch_size"])]) | ||
| if config.get("whisper_compute_type"): | ||
| args.extend(["--whisper_compute_type", config["whisper_compute_type"]]) | ||
| if config.get("whisper_align_model"): | ||
| args.extend(["--whisper_align_model", config["whisper_align_model"]]) | ||
|
|
||
| # Language | ||
| if config.get("language_mode") == "manual" and config.get("language"): | ||
| args.extend(["--language", config["language"]]) | ||
|
|
||
| # Format version | ||
| if config.get("format_version"): | ||
| args.extend(["--format_version", config["format_version"]]) | ||
|
|
||
| # Demucs | ||
| if config.get("demucs_model"): | ||
| args.extend(["--demucs", config["demucs_model"]]) | ||
|
|
||
| # BPM override | ||
| bpm = config.get("bpm_override", "") | ||
| if bpm: | ||
| args.extend(["--bpm", str(bpm)]) | ||
|
|
||
| # Octave shift | ||
| octave = config.get("octave_shift", "") | ||
| if octave: | ||
| args.extend(["--octave", str(octave)]) | ||
|
|
||
| # Denoise | ||
| if config.get("denoise_nr") is not None: | ||
| args.extend(["--denoise_nr", str(config["denoise_nr"])]) | ||
| if config.get("denoise_nf") is not None: | ||
| args.extend(["--denoise_nf", str(config["denoise_nf"])]) | ||
|
|
||
| # Boolean flags (disabled by default in CLI) | ||
| if config.get("disable_separation"): | ||
| args.append("--disable_separation") | ||
| if config.get("disable_quantization"): | ||
| args.append("--disable_quantization") | ||
| if config.get("disable_vocal_center"): | ||
| args.append("--disable_vocal_center") | ||
| if config.get("disable_onset_correction"): | ||
| args.append("--disable_onset_correction") | ||
| if config.get("disable_denoise_track_noise"): | ||
| args.append("--disable_denoise_track_noise") | ||
| if not config.get("hyphenation", True): | ||
| args.append("--disable_hyphenation") | ||
| if not config.get("create_karaoke", True): | ||
| args.append("--disable_karaoke") | ||
|
|
||
| # Boolean flags (enabled by CLI flag) | ||
| if config.get("keep_cache"): | ||
| args.append("--keep_cache") | ||
| if config.get("create_plot"): | ||
| args.append("--plot") | ||
| if config.get("create_midi"): | ||
| args.append("--midi") | ||
| if config.get("create_audio_chunks"): | ||
| args.append("--create_audio_chunks") | ||
| if config.get("keep_numbers"): | ||
| args.append("--keep_numbers") | ||
| if config.get("force_cpu"): | ||
| args.append("--force_cpu") | ||
| if config.get("force_whisper_cpu"): | ||
| args.append("--force_whisper_cpu") | ||
|
|
||
| # Experimental features | ||
| if config.get("syllable_split"): | ||
| args.append("--syllable_split") | ||
| if config.get("vocal_gap_fill"): | ||
| args.append("--vocal_gap_fill") | ||
|
|
||
| # LLM correction | ||
| if config.get("llm_correct"): | ||
| args.append("--llm_correct") | ||
| if config.get("llm_api_base_url"): | ||
| args.extend(["--llm_api_base_url", config["llm_api_base_url"]]) | ||
| if config.get("llm_api_key"): | ||
| args.extend(["--llm_api_key", config["llm_api_key"]]) | ||
| if config.get("llm_model"): | ||
| args.extend(["--llm_model", config["llm_model"]]) | ||
|
|
||
| # Paths | ||
| if config.get("musescore_path"): | ||
| args.extend(["--musescore_path", config["musescore_path"]]) | ||
| if config.get("ffmpeg_path"): | ||
| args.extend(["--ffmpeg", config["ffmpeg_path"]]) | ||
|
|
||
| return args |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the key exists in config/settings but is missing from build_args mappings.
rg -n -C2 '"calculate_score"' src/gui/config.py src/gui/settings_tab.py src/gui/ultrasinger_runner.py
# Identify the actual score-related CLI argument in the main CLI parser.
rg -n -C2 'add_argument\(|score' src/UltraSinger.pyRepository: MrDix/UltraSinger
Length of output: 3037
🏁 Script executed:
#!/bin/bash
# Search for the getopt options definition and look for calculate_score in argument parsing
rg -n "getopt\(" src/UltraSinger.py -A 30 | head -50
# Search for all CLI option definitions in UltraSinger.py
rg -n "calculate_score" src/UltraSinger.py
# Look for the long_options string in argument parsing
rg -n "long_options\s*=" src/UltraSinger.py -A 50 | head -80Repository: MrDix/UltraSinger
Length of output: 1626
🏁 Script executed:
#!/bin/bash
# Find where short and long variables are defined in the argument parsing section
rg -n "^\s*(short|long)\s*=" src/UltraSinger.py
# Also search for long_options definition
rg -n "long_options|short_options" src/UltraSinger.pyRepository: MrDix/UltraSinger
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Read the long options definition
sed -n '1061,1150p' src/UltraSinger.pyRepository: MrDix/UltraSinger
Length of output: 1129
🏁 Script executed:
#!/bin/bash
# Find where build_args is called/used
rg -n "build_args" src/gui/ultrasinger_runner.py -B 3 -A 3Repository: MrDix/UltraSinger
Length of output: 331
🏁 Script executed:
#!/bin/bash
# Find where build_args is called
rg -n "self\.build_args\|\.build_args" src/gui/ultrasinger_runner.pyRepository: MrDix/UltraSinger
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Read the ultrasinger_runner.py file to find how build_args is used
cat -n src/gui/ultrasinger_runner.py | head -300Repository: MrDix/UltraSinger
Length of output: 11577
🏁 Script executed:
#!/bin/bash
# Search for where build_args or runner.start is called in GUI files
rg -n "build_args|runner\.start" src/gui/ -B 2 -A 2Repository: MrDix/UltraSinger
Length of output: 980
Add missing --calculate_score flag to CLI argument mapping.
The calculate_score setting is collected in the GUI settings but never mapped to a CLI argument in build_args(). This means the toggle is ineffective when running conversions—the flag will always use its default value (False) regardless of the user's selection.
Add the mapping to build_args():
Suggested fix
# After the LLM correction block, add:
if config.get("calculate_score"):
args.append("--calculate_score")Also add "calculate_score" to the long options list in UltraSinger.py (around line 1062) and handle it in the argument parsing switch statement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gui/ultrasinger_runner.py` around lines 172 - 275, Missing mapping for
the calculate_score GUI setting: update build_args in ultrasinger_runner.py to
append "--calculate_score" when config.get("calculate_score") is truthy (add
this check after the LLM correction block inside build_args), and then add the
corresponding long option and parser handling in UltraSinger.py by adding
"calculate_score" to the long-options list and handling it in the argument
parsing switch/case so the CLI recognizes and sets the calculate_score behavior.
Comment on lines
+13
to
+14
| if primary: | ||
| self.setObjectName("primaryButton") |
There was a problem hiding this comment.
primary=True has no visual effect right now.
Line 14 sets primaryButton, but src/gui/resources/styles.qss (Lines 1-250) has no #primaryButton rule, so primary/non-primary buttons render the same.
🎨 Suggested wiring in stylesheet
+/* Primary CTA button */
+QPushButton#primaryButton {
+ background-color: `#e91e63`;
+ color: `#ffffff`;
+ border: 1px solid rgba(255,255,255,0.08);
+ border-radius: 8px;
+ padding: 8px 14px;
+ font-weight: 600;
+}
+
+QPushButton#primaryButton:hover {
+ background-color: `#f06292`;
+}
+
+QPushButton#primaryButton:pressed {
+ background-color: `#c2185b`;
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gui/widgets/animated_button.py` around lines 13 - 14, The primary flag
currently sets the Qt object name via setObjectName("primaryButton") in
AnimatedButton (primary) but the app stylesheet (styles.qss) has no
corresponding `#primaryButton` rule, so change the stylesheet or the widget
naming: add a `#primaryButton` selector block to src/gui/resources/styles.qss with
the visual rules you want for primary buttons (colors, border, hover, pressed
states) so the setObjectName("primaryButton") call takes effect, or
alternatively change the AnimatedButton code to set an object name that matches
an existing rule in styles.qss; ensure the object name string used in
setObjectName(...) and the selector in styles.qss are identical.
- ultrasinger_runner: non-blocking cancel via daemon thread - file_drop_zone: accessible name + event.accept() on keypress - settings_card: proper widget cleanup in remove_last_item - config: validate JSON is dict before merging - preferences_tab: guard export_netscape with try/except - queue_tab: reset status label style at conversion start - main_window: dynamic version from importlib.metadata - browser_tab: accessible names for nav buttons, use default UA Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/gui/ultrasinger_runner.py (2)
183-286:⚠️ Potential issue | 🟠 Major
calculate_scoreis still dead config.
build_args()drops thecalculate_scorekey entirely, andsrc/UltraSinger.pyalso has no matching CLI flag in the provided option list. Any GUI or persisted config value for this setting is silently ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/ultrasinger_runner.py` around lines 183 - 286, build_args currently ignores the calculate_score setting so any GUI/persisted value is dropped; add handling in build_args to include the corresponding CLI flag (e.g. check config.get("calculate_score") and append "--calculate_score" or "--no-calculate_score" as your CLI expects) and also add the matching option parsing/flag in the CLI entry (update UltraSinger.py to declare/consume the --calculate_score flag in the option list/argparse setup) so the GUI config and CLI are consistent; update references to build_args and UltraSinger.py option list/argparse to ensure the flag name matches exactly.
48-66:⚠️ Potential issue | 🟠 MajorCancellation is still racy around process startup and exit.
cancel()can flip_cancelledbefore_processis assigned, butrun()only checks the flag before buildingcmd. In that window the subprocess still starts, and the finalif self._cancelledthen rewrites even a normal exit to-2. Downstream,QueueTabcan show “Cancelled” after the conversion actually ran and produced output. Re-check afterPopen()or synchronize_cancelledand_processso only a live process that was actually terminated is reported as cancelled.Also applies to: 92-96, 98-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/ultrasinger_runner.py` around lines 48 - 66, Rework run() and cancel() synchronization so cancellation is only reported when a live subprocess was actually terminated: assign self._process immediately from subprocess.Popen in run(), then re-check self._cancelled right after Popen; if True, terminate/kill that self._process, mark that you terminated it (e.g. _terminated_by_cancel flag) and emit finished(-2) and return. Also update the final exit handling in run() (the block that currently inspects exit codes around _process.wait()/return) to only emit "Cancelled" / finished(-2) when the process was actually terminated by your cancel path (check the _terminated_by_cancel flag or equivalent), and ensure cancel() sets _cancelled and, if self._process is already set, terminates it immediately. Apply the same pattern to the other similar blocks referenced (around lines 92-96 and 98-117) so cancellation is deterministic.src/gui/preferences_tab.py (1)
3-3:⚠️ Potential issue | 🟠 MajorAdd missing
loggingimport before using it in the error handler.Line 189 calls
logging.getLogger(__name__).error(...)in theexcept OSErrorblock, but the module never importslogging. A cookie export failure will raiseNameErrorinstead of being logged.🛠️ Minimal fix
+import logging from pathlib import PathThen replace the inline call on line 189 with:
- logging.getLogger(__name__).error( + logger.error(And add after imports:
+logger = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/preferences_tab.py` at line 3, Add "import logging" to the module imports, then update the exception handler that currently calls logging.getLogger(__name__).error(...) inline (in the export/cookie failure block) to use the imported logging: call logging.getLogger(__name__).error with the same message and include exc_info=True to ensure the caught OSError details are logged; this affects the exception block in the function handling cookie export in preferences_tab.py.
🧹 Nitpick comments (1)
src/gui/widgets/file_drop_zone.py (1)
101-103: Consider deduplicating drag-over styling refresh.The unpolish/polish block is repeated in three places. A small helper (e.g.,
_set_drag_over(active: bool)) will reduce drift and simplify future changes.Also applies to: 108-110, 113-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/file_drop_zone.py` around lines 101 - 103, The drag-over styling refresh is duplicated where you call self.setProperty("dragOver", ...) followed by self.style().unpolish(self) and self.style().polish(self); add a small helper method (e.g., _set_drag_over(self, active: bool)) in file_drop_zone.py that sets self.setProperty("dragOver", active) and performs self.style().unpolish(self); self.style().polish(self), then replace the three duplicate blocks (the occurrences using self.setProperty("dragOver", True/False") and unpolish/polish) with calls to _set_drag_over(True) or _set_drag_over(False) from the existing event handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/preferences_tab.py`:
- Around line 87-89: The code currently calls llm_card.add_row("API Key",
QWidget()); llm_card.remove_last_item(); llm_card.add_layout(key_row) which
drops the "API Key" label and leaves the custom key controls unlabeled; fix by
keeping the label and attaching the key_row directly instead of removing it —
e.g., replace that sequence with a single call that adds the labeled layout (use
llm_card.add_row("API Key", key_row) or the equivalent API to add the key_row as
the widget for the "API Key" label) so the API key controls remain labeled and
accessible (target symbols: llm_card, add_row, remove_last_item, add_layout,
key_row).
In `@src/gui/queue_tab.py`:
- Around line 114-125: In start_conversion(), reset the elapsed display before
starting timers so the previous run's duration doesn't flash; specifically
update the _elapsed_label (e.g. set it to "0s" or an empty duration string) just
prior to calling self._elapsed_timer.start() and self._tick_timer.start(),
ensuring you reference the existing members _elapsed_label, _elapsed_timer, and
_tick_timer in the start_conversion() method.
In `@src/gui/widgets/file_drop_zone.py`:
- Around line 56-69: The widget currently allows unsupported file types because
_open_file_dialog(), drag/drop handlers, and set_file() emit file paths without
a centralized extension check; add a single helper (e.g.,
_validate_and_set_file(path: str) or _is_supported_extension(path: str)) that
checks the file extension against the widget's supported list and only calls
set_file(path) and file_selected.emit(path) when valid; update
_open_file_dialog(), the drop handler, and any callers that currently call
set_file() or file_selected.emit() directly to use this helper so unsupported
files are rejected before being set or emitted (refer to set_file,
_open_file_dialog, and file_selected.emit in the diff).
- Around line 70-71: mousePressEvent currently opens the file picker for any
mouse button; update the mousePressEvent handler to only call
_open_file_dialog() when the event button is the left button (e.g.,
event.button() == Qt.LeftButton) and otherwise delegate to the base
implementation via super().mousePressEvent(_event) so right/middle clicks behave
normally; ensure to import/qualify Qt if needed and reference the existing
mousePressEvent and _open_file_dialog symbols.
---
Duplicate comments:
In `@src/gui/preferences_tab.py`:
- Line 3: Add "import logging" to the module imports, then update the exception
handler that currently calls logging.getLogger(__name__).error(...) inline (in
the export/cookie failure block) to use the imported logging: call
logging.getLogger(__name__).error with the same message and include
exc_info=True to ensure the caught OSError details are logged; this affects the
exception block in the function handling cookie export in preferences_tab.py.
In `@src/gui/ultrasinger_runner.py`:
- Around line 183-286: build_args currently ignores the calculate_score setting
so any GUI/persisted value is dropped; add handling in build_args to include the
corresponding CLI flag (e.g. check config.get("calculate_score") and append
"--calculate_score" or "--no-calculate_score" as your CLI expects) and also add
the matching option parsing/flag in the CLI entry (update UltraSinger.py to
declare/consume the --calculate_score flag in the option list/argparse setup) so
the GUI config and CLI are consistent; update references to build_args and
UltraSinger.py option list/argparse to ensure the flag name matches exactly.
- Around line 48-66: Rework run() and cancel() synchronization so cancellation
is only reported when a live subprocess was actually terminated: assign
self._process immediately from subprocess.Popen in run(), then re-check
self._cancelled right after Popen; if True, terminate/kill that self._process,
mark that you terminated it (e.g. _terminated_by_cancel flag) and emit
finished(-2) and return. Also update the final exit handling in run() (the block
that currently inspects exit codes around _process.wait()/return) to only emit
"Cancelled" / finished(-2) when the process was actually terminated by your
cancel path (check the _terminated_by_cancel flag or equivalent), and ensure
cancel() sets _cancelled and, if self._process is already set, terminates it
immediately. Apply the same pattern to the other similar blocks referenced
(around lines 92-96 and 98-117) so cancellation is deterministic.
---
Nitpick comments:
In `@src/gui/widgets/file_drop_zone.py`:
- Around line 101-103: The drag-over styling refresh is duplicated where you
call self.setProperty("dragOver", ...) followed by self.style().unpolish(self)
and self.style().polish(self); add a small helper method (e.g.,
_set_drag_over(self, active: bool)) in file_drop_zone.py that sets
self.setProperty("dragOver", active) and performs self.style().unpolish(self);
self.style().polish(self), then replace the three duplicate blocks (the
occurrences using self.setProperty("dragOver", True/False") and unpolish/polish)
with calls to _set_drag_over(True) or _set_drag_over(False) from the existing
event handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0135d4dd-a4dc-48f4-8d5a-e89d655f658f
📒 Files selected for processing (8)
src/gui/browser_tab.pysrc/gui/config.pysrc/gui/main_window.pysrc/gui/preferences_tab.pysrc/gui/queue_tab.pysrc/gui/ultrasinger_runner.pysrc/gui/widgets/file_drop_zone.pysrc/gui/widgets/settings_card.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/gui/config.py
- src/gui/main_window.py
- ultrasinger_runner: deterministic cancellation with _terminated_by_cancel flag, re-check after Popen, thread-safe cancel - file_drop_zone: centralized _validate_and_set(), _set_drag_over() helper, left-click only for mousePressEvent - preferences_tab: add logging import, proper API Key label in layout - queue_tab: reset elapsed label at conversion start Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/gui/ultrasinger_runner.py (1)
193-296:⚠️ Potential issue | 🟠 Major
calculate_scoreis still not propagated from GUI config to CLI args.
build_args()maps many settings, but it never appends--calculate_score. Sincesettings_tab.collect_config()andconfig.pyboth includecalculate_score, this toggle remains ineffective at runtime.🛠️ Suggested fix
# LLM correction if config.get("llm_correct"): args.append("--llm_correct") if config.get("llm_api_base_url"): args.extend(["--llm_api_base_url", config["llm_api_base_url"]]) if config.get("llm_api_key"): args.extend(["--llm_api_key", config["llm_api_key"]]) if config.get("llm_model"): args.extend(["--llm_model", config["llm_model"]]) + # Scoring + if config.get("calculate_score"): + args.append("--calculate_score") + # Paths if config.get("musescore_path"): args.extend(["--musescore_path", config["musescore_path"]])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/ultrasinger_runner.py` around lines 193 - 296, The build_args() function in ultrasinger_runner.py never adds the --calculate_score CLI flag so the GUI's calculate_score setting has no effect; update build_args (in the same function) to check config.get("calculate_score") and, when truthy, append the "--calculate_score" flag to args (similar to other boolean flags like "keep_cache" and "create_plot") so the CLI receives the toggle.
🧹 Nitpick comments (1)
src/gui/widgets/file_drop_zone.py (1)
56-76: Keep extension validation invariant inside the public setter.
set_file()is public and currently bypasses extension checks; only_validate_and_set()enforces them. If any caller usesset_file()directly, unsupported files can still enter widget state. Consider validating inset_file()and keeping_validate_and_set()as the emit wrapper.Proposed refactor
- def set_file(self, path: str): + def set_file(self, path: str) -> bool: """Set the file path programmatically.""" + if path: + ext = Path(path).suffix.lower() + if ext not in ALL_EXTENSIONS: + return False self._current_file = path if path: name = Path(path).name self._file_label.setText(name) self._file_label.show() self._text_label.setText("Click to change file") self._icon_label.setText("\U0001F3B5") else: self._file_label.hide() self._text_label.setText("Drag file here or click to select") self._icon_label.setText("\U0001F4C1") + return True def _validate_and_set(self, path: str): """Validate extension, set file, and emit signal.""" - ext = Path(path).suffix.lower() - if ext in ALL_EXTENSIONS: - self.set_file(path) + if self.set_file(path): self.file_selected.emit(path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/file_drop_zone.py` around lines 56 - 76, set_file currently mutates widget state without checking extensions, allowing unsupported files; move the extension check into set_file (use Path(path).suffix.lower() vs ALL_EXTENSIONS) so it rejects/ignores invalid paths and only updates _current_file, _file_label, _text_label, and _icon_label for valid ones, then keep _validate_and_set as a thin wrapper that calls set_file(path) and emits file_selected(path) only if set_file accepted the path (e.g., returns a bool or raises on invalid); ensure callers of set_file receive the same validation behavior as _validate_and_set and preserve symbols: set_file, _validate_and_set, ALL_EXTENSIONS, file_selected, _current_file, _file_label, _text_label, _icon_label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/preferences_tab.py`:
- Around line 114-118: The cookie-action UI should be disabled whenever
self._cookie_manager is missing: update the constructor/init block that
currently checks cookie_manager to also set the cookie action buttons disabled
when cookie_manager is falsy, and only connect signals and call
self._update_cookie_status when a manager exists; modify
self._update_cookie_status to set the enabled/disabled state of the cookie
action buttons (and the self._cookie_status QLabel text) based on
self._cookie_manager being present, and ensure any signal connections use the
same self._cookie_manager instance to avoid silent no-ops; specifically touch
the _cookie_manager variable, the _cookie_status QLabel, the
_update_cookie_status method, and the cookie action button attributes so they
are disabled when no manager and re-enabled when a manager is available.
In `@src/gui/queue_tab.py`:
- Around line 181-186: In _open_output_folder, wrap the subprocess.Popen calls
in a try/except that catches OSError (or Exception) and logs the failure so the
UI callback can't raise unhandled errors; locate the platform branch using the
subprocess.Popen calls in _open_output_folder, execute the appropriate opener
inside the try block, and in except log the exception details (include the
exception object/message) using the module/class logger so the error is recorded
and the UI remains stable.
---
Duplicate comments:
In `@src/gui/ultrasinger_runner.py`:
- Around line 193-296: The build_args() function in ultrasinger_runner.py never
adds the --calculate_score CLI flag so the GUI's calculate_score setting has no
effect; update build_args (in the same function) to check
config.get("calculate_score") and, when truthy, append the "--calculate_score"
flag to args (similar to other boolean flags like "keep_cache" and
"create_plot") so the CLI receives the toggle.
---
Nitpick comments:
In `@src/gui/widgets/file_drop_zone.py`:
- Around line 56-76: set_file currently mutates widget state without checking
extensions, allowing unsupported files; move the extension check into set_file
(use Path(path).suffix.lower() vs ALL_EXTENSIONS) so it rejects/ignores invalid
paths and only updates _current_file, _file_label, _text_label, and _icon_label
for valid ones, then keep _validate_and_set as a thin wrapper that calls
set_file(path) and emits file_selected(path) only if set_file accepted the path
(e.g., returns a bool or raises on invalid); ensure callers of set_file receive
the same validation behavior as _validate_and_set and preserve symbols:
set_file, _validate_and_set, ALL_EXTENSIONS, file_selected, _current_file,
_file_label, _text_label, _icon_label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49dce22b-6bd7-4367-8955-5a1d2c7a612e
📒 Files selected for processing (4)
src/gui/preferences_tab.pysrc/gui/queue_tab.pysrc/gui/ultrasinger_runner.pysrc/gui/widgets/file_drop_zone.py
- preferences_tab: disable cookie buttons when cookie_manager unavailable - queue_tab: wrap _open_output_folder in try/except OSError, add logger Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/gui/preferences_tab.py (2)
1-6: Logger defined between import groups.The logger is initialized at line 6, between the
logging/pathlibimports and the PySide6 imports. While functional, conventionally all imports are grouped together before module-level statements.♻️ Suggested reorder
"""Application preferences and configuration tab.""" import logging from pathlib import Path -logger = logging.getLogger(__name__) - from PySide6.QtCore import Qt from PySide6.QtWidgets import ... ) from .widgets import AnimatedButton, SettingsCard, ToggleSwitch +logger = logging.getLogger(__name__) + class PreferencesTab(QWidget):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/preferences_tab.py` around lines 1 - 6, The module-level logger (logger) is declared between import groups; move the logger initialization so all imports remain grouped together first and then module-level statements follow. Specifically, place the line that defines logger = logging.getLogger(__name__) after all import statements (including the PySide6 imports elsewhere in this file) so imports are contiguous and the logger is declared as a standalone module-level variable.
190-206: Consider providing user feedback on export success/failure.The export operation logs errors but provides no visual feedback to the user. For better UX, consider showing a brief status message or toast notification on success/failure rather than silently logging.
💡 Optional UX enhancement
def _export_cookies(self): if self._cookie_manager: # Prefer UI path, fall back to config cookie_path = self._cookie_path.text() or self._config.get("cookie_file", "") if cookie_path: try: exported = self._cookie_manager.export_netscape(cookie_path) self._cookie_path.setText(str(exported)) + # Optional: Show success feedback + if hasattr(self, '_cookie_status'): + self._cookie_status.setText(f"✅ Cookies exported to {exported.name}") except OSError as e: logger.error( "Failed to export cookies to %s: %s", cookie_path, e, exc_info=True, ) + # Optional: Show error feedback via QMessageBox🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/preferences_tab.py` around lines 190 - 206, The _export_cookies routine currently logs failures via logger but gives no user-visible feedback; update _export_cookies so that after calling self._cookie_manager.export_netscape(cookie_path) it displays a brief success message to the user (e.g., status bar/toast/QMessageBox) and on exception shows a clear error message containing the exception text; use the existing symbols _export_cookies, _cookie_manager, _cookie_path and _config to locate the code and ensure the error branch that currently logs via logger.error also triggers the user-facing failure notification with the same exception details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/preferences_tab.py`:
- Around line 114-118: The bug: _update_cookie_status unconditionally references
self._cookie_status which is only created when cookie_manager is truthy, causing
AttributeError if called with cookie_manager None; fix by either (preferred)
initializing self._cookie_status in the class __init__ (e.g. set
self._cookie_status = QLabel("") and add it to cookie_card unconditionally) so
it always exists, or add a defensive check at the start of _update_cookie_status
(if getattr(self, "_cookie_status", None) is None: return) to avoid accessing a
missing attribute; update references to
cookie_manager.cookies_changed.connect(self._update_cookie_status) to keep
behavior unchanged.
In `@src/gui/queue_tab.py`:
- Line 151: The button enablement currently checks only that
self._current_output_folder is non-empty; change it to enable only when the path
actually exists and is a directory. Update the check in the code that sets
self._open_folder_btn.setEnabled(...) to verify
os.path.isdir(self._current_output_folder) (or
Path(self._current_output_folder).is_dir()), guarding for None/empty string, and
ensure the module imports os or pathlib as needed; this ensures the Open Output
Folder button is enabled only for a valid existing folder.
- Around line 176-190: Replace the platform-specific subprocess logic in
_open_output_folder with PySide6's QDesktopServices.openUrl: import
QDesktopServices and QUrl from PySide6.QtGui (or QtCore/QtGui as appropriate),
check self._current_output_folder exists, build a QUrl using
QUrl.fromLocalFile(self._current_output_folder) and call
QDesktopServices.openUrl(url) to open the folder; remove subprocess and sys
imports and any platform branching so the method consistently uses
QDesktopServices.openUrl with QUrl.fromLocalFile.
---
Nitpick comments:
In `@src/gui/preferences_tab.py`:
- Around line 1-6: The module-level logger (logger) is declared between import
groups; move the logger initialization so all imports remain grouped together
first and then module-level statements follow. Specifically, place the line that
defines logger = logging.getLogger(__name__) after all import statements
(including the PySide6 imports elsewhere in this file) so imports are contiguous
and the logger is declared as a standalone module-level variable.
- Around line 190-206: The _export_cookies routine currently logs failures via
logger but gives no user-visible feedback; update _export_cookies so that after
calling self._cookie_manager.export_netscape(cookie_path) it displays a brief
success message to the user (e.g., status bar/toast/QMessageBox) and on
exception shows a clear error message containing the exception text; use the
existing symbols _export_cookies, _cookie_manager, _cookie_path and _config to
locate the code and ensure the error branch that currently logs via logger.error
also triggers the user-facing failure notification with the same exception
details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e4af703-d14a-4975-9d8e-cef28e76d280
📒 Files selected for processing (2)
src/gui/preferences_tab.pysrc/gui/queue_tab.py
- queue_tab: use QDesktopServices.openUrl instead of subprocess for folder opening, check Path.exists() for open button enablement - preferences_tab: guard _update_cookie_status against missing _cookie_status, move logger after all imports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Split into two services using YAML anchors: - `ultrasinger` (default): CPU mode, works everywhere - `ultrasinger-gpu` (profile: gpu): requires NVIDIA Container Toolkit Usage: `docker compose run ultrasinger ...` for CPU, `docker compose --profile gpu run ultrasinger-gpu ...` for GPU. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Explains CPU vs GPU mode, volume mounts, and cookie usage. Keeps existing container/README.md link for manual setups. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove root docker-compose.yml (was using profiles approach) - Replace old compose-nogpu.yml with compose-cpu.yml (build from source) - Update compose-gpu.yml (build from source, correct volume paths) - Rewrite docker.md: run vs up -d, volumes, cookies, manual build - Rewrite podman.md: Bash + PowerShell examples, run vs detached - Update container/README.md with quick start - Simplify main README container section Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- New "Run (GUI)" section: install, start, feature overview - Add GUI to table of contents - Clarify: GUI is native desktop app, not available in containers - Remove misleading "GUI mode" references from container docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- API keys stored in system keyring (Windows Credential Manager, macOS Keychain, Linux Secret Service) instead of config.json - Environment variable fallback: ULTRASINGER_LLM_API_KEY - Legacy migration: keys in config.json auto-migrate to keyring on first load - Directory permissions: ~/.ultrasinger/ restricted to owner-only (chmod 0o700 on POSIX, icacls on Windows) - config.json file permissions: 0o600 on POSIX - Secret keys stripped from JSON before write - Preferences tab shows keyring backend status - 9 new tests covering priority chain, migration, stripping, ACLs - keyring>=25.0 added to gui extras Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Sidebar now has a file drop zone at top — drop file, convert button appears immediately. No need to navigate to Settings first. - YouTube flow: click Convert in browser → URL appears in sidebar with convert button. One click to start. - Settings tab: removed input section (YouTube/Local File radio, URL field, FileDropZone). Now only contains conversion options. - Input source management moved from SettingsTab to Sidebar. Before: Settings → Local File → Drop → Start (4 steps) After: Drop in sidebar → Start (2 steps) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
YouTube is a single-page app — internal navigation (clicking videos, search results) only fires urlChanged, not loadFinished. Re-inject the Convert button overlay after each SPA navigation with a short delay to let YouTube's rendering settle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace manual runJavaScript() calls with QWebEngineScript (Chromium's user-script API, equivalent to Tampermonkey). The script is now automatically injected on every full page load by the engine itself. Additional robustness for YouTube's SPA architecture: - Listen for YouTube's custom `yt-navigate-finish` event - Periodic 3s poll as fallback if YouTube removes the element - z-index raised to max (2147483647) to stay above YouTube's UI - Use addEventListener instead of onX properties - console.log for debugging Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Browser tab: - Convert overlay only appears on /watch?v= video pages, hidden elsewhere - URL bar is now editable — paste YouTube URLs and press Enter to navigate - URL validation: only youtube.com, youtu.be, music.youtube.com accepted Settings tab: - Reset buttons (↺) next to every setting to restore defaults - Scroll-safe spin boxes: mouse wheel only changes value when focused (prevents accidental changes while scrolling the settings page) - All ComboBoxes also scroll-safe - Info labels are now text-selectable (copy URLs like console.groq.com) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Critical bug: when clicking Convert on a YouTube video, the URL often contains &list= parameters (autoplay mix). yt-dlp interprets this as a playlist and downloads dozens of videos instead of the selected one. Fix (defense in depth): - JS overlay: build a clean URL with only ?v= parameter - Python: _clean_youtube_url() strips playlist params as safety net Cancel fix: - Create subprocess in new process group (CREATE_NEW_PROCESS_GROUP on Windows, start_new_session on Unix) - Kill entire process tree on cancel: taskkill /F /T on Windows, os.killpg on Unix — ensures yt-dlp/ffmpeg children are terminated Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Queue → Console: - Rename "Queue" sidebar entry and tab header to "Console" YouTube URL display: - Show compact "YT: /watch?v=abc123" instead of full URL in sidebar Fix output folder bug: - Remove "Output Folder" from Settings tab (was duplicating Preferences) - Only Preferences tab manages output_folder — no more conflicts - Rename "Default Output Folder" → "Output Folder" in Preferences Fix LLM settings duplication: - Remove API Base URL and API Key from Settings tab - Settings only has LLM toggle + model selector - API credentials are managed exclusively in Preferences - Info label directs users to Preferences for API configuration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Accidentally removed during Output Folder cleanup — the Paths section still needs QPushButton (Browse) and QFileDialog. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace all visible and code-level platform name references with neutral terms (Video, video URL, etc.) to avoid trademark issues - Replace Unicode hieroglyph icons (↺, ↻, ◀, ▶) with Qt standard icons (SP_ArrowBack, SP_ArrowForward, SP_BrowserReload, SP_DialogResetButton) for clean platform-native appearance - Fix output folder Browse dialog rejecting valid paths on Windows by using DontUseNativeDialog option - Rename internal APIs: youtube_cookie_count → video_cookie_count, has_youtube_cookies → has_video_cookies, set_youtube_input → set_video_input, _YOUTUBE_URL_RE → _VIDEO_URL_RE, etc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
MrDix
changed the title
MrDix
added a commit
that referenced
this pull request
Mar 19, 2026
Complete PySide6 desktop GUI with sidebar navigation, dark theme, embedded video browser (QWebEngineView) with Convert overlay, system keyring for API keys, cookie management with Netscape export, all CLI parameters as form controls, real-time conversion log, and Docker/Podman compose configurations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 19, 2026
This was referenced Mar 26, 2026
MrDix
added a commit
that referenced
this pull request
Jul 4, 2026
Complete PySide6 desktop GUI with sidebar navigation, dark theme, embedded video browser (QWebEngineView) with Convert overlay, system keyring for API keys, cookie management with Netscape export, all CLI parameters as form controls, real-time conversion log, and Docker/Podman compose configurations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
uv sync --extra guito install, no impact on CLI usageArchitecture
Key Features
Install & Launch
Test plan