Conversation
Rewrite of MacAlpha v0.1. Same idea, but the features it advertised now actually run. Fixed (all were live in v0.1): - Drag & drop was never wired up; tkinterdnd2 was in requirements but never imported. The drop zone was a clickable rectangle. - "Preserve EXIF & color profiles" was ignored. piexif was imported and unused; metadata was dropped from every output file. - MAX_THREADS: 8 did nothing. run_batch_conversion() existed but was never called; the progress screen converted serially in a for loop. - Time remaining was permanently "Calculating..."; start_time was assigned and never read. Results always showed "Time: --". - Cancel broke the loop and then reported "Conversion Complete!". - Errors were recorded and never displayed anywhere. - One Label per file meant a frozen window on large batches. - setup.py hardcoded assets/icon.icns, which was never committed, so every py2app build failed. - The CI workflow cd'd into ConvertImagesToWebP-MacAlpha/, a folder not in this repo, so it had never once produced a bundle. - Return started a conversion while you were typing in a settings field (focus_get() returns the inner tkinter.Entry, not CTkEntry). - The log used font family "monospace", which Tk does not define; it silently resolved to Arial, so columns never aligned. New: - Output formats WebP / AVIF / JPEG / PNG, filtered to what the installed Pillow can actually encode. - Presets: Web, Balanced, Archive, Smallest. - Resize by longest edge, width, height or megapixels. Never upscales. - Destination control (subfolder / chosen folder / in place) and an existing-file policy (skip / overwrite / rename). - Preflight count and total size before the run starts. - Live ETA and images/sec, working cancel that reports partial results. - Optional GPS-only metadata stripping. - System / Light / Dark, remembered between launches. - Keyboard shortcuts. Structure: core/ has no UI imports, so the engine is scriptable and testable without a display. gui/theme.py holds design tokens as (light, dark) pairs instead of greys hardcoded across fourteen call sites. Behaviour worth knowing: - The output folder is excluded from scans, so converting a folder twice no longer re-converts its own results. - A failed write is deleted rather than left truncated. - With metadata off, pixels are converted to sRGB so untagged output does not shift color. Tests: tests/test_engine.py (12 checks, no display) and tests/test_gui_boot.py (builds the real window, runs a real conversion through the Tk event loop). CI runs the engine on Linux, the GUI on macOS and Windows, and builds Intel and Apple Silicon bundles separately -- py2app bundles the interpreter it runs with, so a single runner produces an app that will not launch on the other architecture. Verified on Windows 11 / Python 3.12 / Tk 8.6 / CustomTkinter 6.0. Not yet verified on macOS; no Mac was available. CI covers the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reached
Next review available in: 30 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughWebP Studio 2.0 replaces the legacy macOS converter with a cross-platform batch conversion engine, configurable settings, a new GUI workflow, concurrent processing, startup diagnostics, automated tests, and architecture-specific macOS packaging. ChangesWebP Studio 2.0
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant HomeScreen
participant Runner
participant ProgressScreen
participant ResultsScreen
User->>HomeScreen: Select or drop image sources
HomeScreen->>Runner: Scan sources
Runner-->>HomeScreen: Return Scan
User->>HomeScreen: Start conversion
HomeScreen->>ProgressScreen: Start Scan
ProgressScreen->>Runner: Run conversion in background
Runner-->>ProgressScreen: Send progress and file results
Runner-->>ProgressScreen: Send completion results
ProgressScreen->>ResultsScreen: Display conversion results
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 14
🧹 Nitpick comments (11)
core/runner.py (2)
175-179: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTake a snapshot of
settingsinstead of mutating and reading the shared instance.
run()callsself.settings.clamp(), which mutates the sameSettingsobject the GUI owns.gui/panel.pyedits that instance in response to user input, andgui/panel.py:68also callsclamp()on it.Worker threads read
self.settingsthroughout the batch:_claim_destinationreadsoutput_suffix()andon_existing, andconvert_filereads the format, quality, and geometry fields. If any edit reaches the object while workers run, files in one batch encode with different settings. Nothing in the engine enforces the "progress screen is showing, so no edits happen" assumption.Copy the settings in
__init__so the batch is self-consistent by construction.♻️ Proposed change
+import copyself.scan = scan - self.settings = settings + # Snapshot: the GUI keeps editing its own Settings instance, and a + # batch must encode every file with one consistent configuration. + self.settings = copy.deepcopy(settings) + self.settings.clamp()def run(self) -> list[FileResult]: """Blocking. Call from a worker thread if you have a UI.""" - self.settings.clamp() self._started = time.monotonic()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/runner.py` around lines 175 - 179, Copy the incoming Settings object in the runner’s __init__ instead of retaining the GUI-owned instance, then use that snapshot through run(), _claim_destination, and convert_file. Preserve the existing clamp behavior on the runner-owned copy so worker reads remain consistent throughout the batch.
244-249: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe claim lock is held across filesystem syscalls.
self._lockwraps the whole loop, andcandidate.exists()runs inside it._recordneeds the same lock. Withworker_count()up to 16 threads on a network share or a cold spinning disk, every claim serializes behind onestatround trip, and progress reporting waits behind it too.Consider a two-phase claim: reserve the name string under the lock, and perform the
exists()probe outside it, retrying if the reservation loses a race.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/runner.py` around lines 244 - 249, Refactor the claim loop around the lock in the runner method so the lock only protects reserving and recording the candidate name, not candidate.exists() filesystem probes. Reserve the lowercased name under self._lock, check existence outside the lock, and retry when the reservation loses a race; preserve _record’s locking and existing collision/index behavior.tests/test_engine.py (1)
92-109: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for
strip_gps.
test_metadatacoverskeep_metadatain both states. It does not coverstrip_gps. That path is the privacy-relevant one:core/imaging.py:70-84must remove the GPS IFD and keep the remaining EXIF, and it must drop all EXIF whenpiexifis absent.A silent regression there ships coordinates the user asked to remove, and no current test detects it.
💚 Suggested additional test
def test_strip_gps(tmp: Path): """GPS must go; the rest of the EXIF must stay.""" from core.imaging import PIEXIF_OK if not PIEXIF_OK: print(" strip gps skipped (piexif absent)") return import piexif src = tmp / "in" / "gps.jpg" src.parent.mkdir(parents=True, exist_ok=True) Image.new("RGB", (60, 60), "blue").save(src) piexif.insert(piexif.dump({ "0th": {piexif.ImageIFD.Make: b"SelfCheckCamera"}, "GPS": {piexif.GPSIFD.GPSLatitudeRef: b"N"}, }), str(src)) out = tmp / "out" / "nogps.webp" convert_file(src, out, Settings(keep_metadata=True, strip_gps=True)) with Image.open(out) as img: raw = img.info.get("exif") assert raw, "all EXIF was dropped instead of only GPS" data = piexif.load(raw) assert not data["GPS"], data["GPS"] assert data["0th"][piexif.ImageIFD.Make] == b"SelfCheckCamera" print(" strip gps ok")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_engine.py` around lines 92 - 109, Add a test_strip_gps test alongside test_metadata that skips when PIEXIF_OK is false, creates an image containing both GPS and non-GPS EXIF, and converts it with Settings(keep_metadata=True, strip_gps=True). Verify the output retains EXIF and the Make field while its GPS IFD is empty, covering the piexif-backed privacy path.core/config.py (2)
138-149: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle write failures in
save(), and narrow theexceptinload().
save()has no error handling.gui/app.py:99-104callsself.settings.save()inside a Tk callback. If the disk is full or the config directory is read-only, theOSErrorescapes into the Tk event loop and surfaces as an unhandled-exception traceback during a theme switch.
load()catches bareException(Ruff BLE001). The intent is corrupt-config tolerance, but the current form also hides aTypeErrorcaused by a future field rename, which would silently reset every user setting.♻️ Proposed change
def save(self) -> None: # Write-then-rename: a crash mid-write can't leave a truncated config. - tmp = CONFIG_FILE.with_suffix(".json.tmp") - tmp.write_text(json.dumps(self.to_dict(), indent=2), encoding="utf-8") - tmp.replace(CONFIG_FILE) + tmp = CONFIG_FILE.with_suffix(".json.tmp") + try: + tmp.write_text(json.dumps(self.to_dict(), indent=2), encoding="utf-8") + tmp.replace(CONFIG_FILE) + except OSError: + # Settings are a convenience, not the product. A run must not die + # because the config volume is full or read-only. + tmp.unlink(missing_ok=True) `@classmethod` def load(cls) -> "Settings": try: return cls.from_dict(json.loads(CONFIG_FILE.read_text(encoding="utf-8"))) - except Exception: + except (OSError, ValueError, TypeError): return cls()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/config.py` around lines 138 - 149, Update Settings.save() to catch filesystem OSError failures from writing or replacing the temporary config file, preserving the callback without an unhandled exception. Narrow Settings.load() to catch only the JSON parsing and file-read errors needed for corrupt or unavailable config tolerance, allowing TypeError from cls.from_dict() to propagate instead of silently resetting settings.Source: Linters/SAST tools
34-53: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid directory creation and path freezing at import time.
CONFIG_FILEis evaluated at import, soconfig_dir()creates a directory as a side effect ofimport core.config. Two consequences follow:
- If
mkdirfails (read-only volume, sandboxed container, restricted CI runner), the import raisesOSError. The engine and the GUI then fail to start, even for runs that never touch settings.WEBP_STUDIO_CONFIG_DIRis read once. A test that sets the variable after the first import still writes to the real user config path.Resolve the path lazily instead.
♻️ Lazy path resolution
-CONFIG_FILE = config_dir() / "settings.json" +def config_file() -> Path: + return config_dir() / "settings.json"Then use
config_file()insidesave()andload():def save(self) -> None: target = config_file() tmp = target.with_suffix(".json.tmp") tmp.write_text(json.dumps(self.to_dict(), indent=2), encoding="utf-8") tmp.replace(target)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/config.py` around lines 34 - 53, Replace the import-time CONFIG_FILE initialization with a config_file() function that resolves the current config_dir() lazily and creates directories only when settings are accessed. Update the settings save() and load() methods to call config_file() for their target path, preserving existing read/write behavior and allowing WEBP_STUDIO_CONFIG_DIR changes after import.core/imaging.py (1)
17-19: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer a high ceiling over disabling the decompression-bomb guard.
Image.MAX_IMAGE_PIXELS = Noneremoves the limit for the whole process. The stated use case is 100 MP scans, which a raised ceiling also covers. With no limit, one crafted or corrupt file that declares a multi-gigapixel canvas makes Pillow attempt the full allocation. The process is then killed by the OS, and the entire batch is lost rather than one file.Drag and drop accepts files the user did not create, so "own disk" does not guarantee trusted input.
🛡️ Proposed change
-# We routinely handle 100 MP camera scans; the decompression-bomb guard is for -# untrusted input, and these files come from the user's own disk. -Image.MAX_IMAGE_PIXELS = None +# We routinely handle 100 MP camera scans, so Pillow's default ~89 MP guard is +# too low. Raise it rather than remove it: an unbounded limit turns one crafted +# file into an OOM kill that loses the whole batch. +Image.MAX_IMAGE_PIXELS = 1_000_000_000🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/imaging.py` around lines 17 - 19, Replace the global Image.MAX_IMAGE_PIXELS = None assignment with a finite high ceiling that accommodates the supported 100 MP scans while retaining Pillow’s decompression-bomb protection. Keep the change scoped to the imaging initialization near Image.MAX_IMAGE_PIXELS.gui/widgets.py (1)
143-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
intcall.
round(raw)with a single argument already returns anint. Ruff flags the wrappingint()as unnecessary (RUF046).♻️ Proposed simplification
def _changed(self, raw: float) -> None: - value = int(round(raw)) + value = round(raw) self.value_label.configure(text=f"{value}{self._suffix}") self._on_change(value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/widgets.py` around lines 143 - 146, Remove the redundant int() wrapper in _changed and assign the rounded raw value directly, preserving the existing label update and _on_change behavior.Source: Linters/SAST tools
gui/screens/results.py (1)
159-169: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
os.startfileon Windows; the injection findings are false positives.The static analysis rules S603, S607, and
subprocess-from-requestreport command injection here. That is not accurate: the call uses the argument-list form withshell=False, so no shell parsesfolder. The remaining concerns are real but smaller.
- On Windows,
explorermishandles some paths passed as a bare argument and returns exit code 1 on success.os.startfileis the supported API.- The executables resolve through
PATH.- If
output_folderisNone, the button does nothing and gives no feedback.♻️ Proposed change
def open_output(self) -> None: folder = self.output_folder if not folder or not folder.exists(): return system = platform.system() if system == "Darwin": subprocess.run(["open", str(folder)], check=False) elif system == "Windows": - subprocess.run(["explorer", str(folder)], check=False) + os.startfile(str(folder)) # noqa: S606 - documented Windows shell API else: subprocess.run(["xdg-open", str(folder)], check=False)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/screens/results.py` around lines 159 - 169, Update open_output to use os.startfile(str(folder)) on Windows instead of invoking explorer via subprocess. Preserve the existing early return and platform-specific behavior for macOS and other systems, and keep subprocess executable resolution unchanged outside Windows.Source: Linters/SAST tools
gui/screens/home.py (1)
265-269: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpose a public reload method on
SettingsPanel.
on_showcallsself.panel._reload_controls(). A screen reaches into a private method of another module. Rename the panel method toreload_controlsand keep a thin private alias if internal callers need it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/screens/home.py` around lines 265 - 269, Rename SettingsPanel’s _reload_controls method to the public reload_controls method and update on_show to call reload_controls instead of the private member. Preserve a thin _reload_controls alias only where existing internal callers still require it.gui/screens/progress.py (1)
120-144: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded
LogView.appendloops ingui/screens/progress.pyandgui/screens/results.py. Both sites append one line per result to aLogViewthat keeps onlyMAX_LINES(400). Every extra append performs an insert, a delete, asee("end"), and twoconfigurecalls on the Tk thread, and the result is discarded. A large batch therefore blocks the UI to render text that no user ever sees. A batched append API onLogView, or a cap at each call site, fixes both.
gui/screens/progress.py#L120-L144: collect the drainedfileevents into a list and render only the lastLogView.MAX_LINESentries after the drain loop.gui/screens/results.py#L137-L156: build the failed, skipped, and note lines into one list, truncate it toLogView.MAX_LINES, and append the truncated list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/screens/progress.py` around lines 120 - 144, The unbounded LogView.append loops should be capped before rendering. In gui/screens/progress.py#L120-L144, collect drained file-event lines, then append only the last LogView.MAX_LINES entries after the drain loop; in gui/screens/results.py#L137-L156, combine failed, skipped, and note lines into one list, truncate it to LogView.MAX_LINES, and append the truncated list.gui/panel.py (1)
218-225: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReload the entry before notifying, or keep the two calls consistent.
_set_resize_modecalls_changed()first and_reload_resize_entry()second._changed()already calls_sync_conditional_rows(), which calls_reload_resize_entry()when the row is visible. The extra call is redundant and the ordering differs from_commit_resize_valueonly by chance. Also, switching frommegapixelsto a pixel mode keeps a small value such as100, whichclamp()accepts as a 100-pixel limit.♻️ Proposed simplification
def _set_resize_mode(self, label: str) -> None: self.settings.resize_mode = RESIZE_BY_LABEL[label] if self.settings.resize_mode == "megapixels" and self.settings.resize_value > 500: self.settings.resize_value = 12.0 - elif self.settings.resize_mode != "megapixels" and self.settings.resize_value < 16: + elif self.settings.resize_mode != "megapixels" and self.settings.resize_value < 512: self.settings.resize_value = 2048.0 self._changed() - self._reload_resize_entry()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/panel.py` around lines 218 - 225, Update _set_resize_mode to normalize resize_value when switching modes, including replacing small values such as 100 with the valid pixel-mode default rather than allowing them through clamp(). Make _reload_resize_entry and _changed consistent with _commit_resize_value, removing the redundant reload while preserving the visible-row synchronization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Around line 50-53: Replace the unsupported macos-13 runner with macos-15-intel
in .github/workflows/build.yml lines 50-53. Update the Intel runner label in
README.md lines 82-86, and correct the CI status statement in README.md lines
152-159 to reflect the updated workflow.
- Around line 1-13: Set top-level workflow permissions to contents: read in the
Build workflow, and update all three actions/checkout@v4 steps to use
persist-credentials: false. Preserve the existing checkout behavior and job
configuration.
In `@core/config.py`:
- Around line 170-197: Make preset application capability-aware: update the
"Smallest" preset or apply_preset to avoid selecting AVIF when avif is absent
from available_output_formats(), falling back to webp while preserving the other
preset settings. In core/config.py lines 170-197, implement the preset gate or
fallback; in core/imaging.py lines 39-50, verify the Pillow 11.3
features.check("avif") probe name and make the webp and avif probes consistent
so available_output_formats() is authoritative.
- Line 110: Update the quality fallback in the configuration normalization logic
to use the documented Balanced preset default of 85 instead of 82. Preserve the
existing _clamp_int bounds and valid-value behavior.
In `@core/imaging.py`:
- Around line 229-238: Add a pre-encode dimension check in the image-processing
flow before img.save, using the selected output format’s maximum dimensions
(including WebP’s 16383 px per side and AVIF’s limits). Either downscale
oversized images and record the adjustment through Encoded.note, or raise an
actionable format-specific error instead of allowing the encoder to fail with
its raw message.
- Around line 182-207: Clarify the AVIF lossless behavior in the user-facing UI
documentation or tooltip associated with the AVIF quality/lossless settings,
stating that Pillow implements it as quality=100 rather than a dedicated
lossless mode. Alternatively, if the product promises true lossless AVIF, update
the AVIF handling in _save_kwargs to use an explicit lossless encoder path; do
not change the existing effort-to-speed mapping.
In `@core/runner.py`:
- Around line 251-261: Update the overwrite path in convert_file and its
_process error handling to encode into a temporary .part file in the same
folder, then atomically replace the existing destination only after encoding
succeeds. Preserve direct safe output handling for new files, remove the
temporary file on failure, and ensure overwrite failures never unlink the
previously valid destination.
- Around line 182-191: Ensure run() always invokes on_finish, including when
pool.map or _process raises, by moving completion handling into a finally path
while preserving elapsed-time calculation and exception propagation. Update the
ThreadPoolExecutor.map comment to accurately describe eager task submission and
cancellation via _process checking self._cancel. Harden _process cleanup around
destination.unlink so cleanup failures cannot mask the original processing
error.
In `@gui/app.py`:
- Around line 175-182: Update ProgressScreen.start to retain the thread returned
by Runner.start_background(), then update on_close to cancel the active runner
and briefly join its stored worker thread with a short timeout before calling
self.destroy(). Preserve the existing settings save flow and avoid blocking
indefinitely when the worker does not stop promptly.
In `@gui/screens/home.py`:
- Around line 156-165: Update set_sources so that when filtering leaves no
existing paths, it updates the summary label with a user-facing message before
returning; preserve the current source assignment and rendering flow when at
least one valid path remains.
- Around line 232-235: Update _settings_changed so encoding-only changes such as
Quality and Encoder-effort do not trigger _rescan, while destination- and
exclusion-affecting changes still do. Debounce eligible rescans using the UI
scheduler’s after_cancel/after pattern, retaining and cancelling the pending
callback before scheduling a new one to prevent repeated filesystem walks during
slider dragging.
In `@gui/screens/results.py`:
- Around line 171-188: Update save_log to catch OSError around
Path(target).write_text, and report the failure by configuring save_log_button
with an appropriate error message instead of allowing the exception to escape.
Keep the existing “Saved ✓” text and delayed reset only for successful writes.
In `@README.md`:
- Line 156: Update the README.md environment table entry to document
CustomTkinter as >=5.2.2, matching the customtkinter requirement in
requirements.txt; do not leave the documented 6.0 version inconsistent with the
installed dependency.
In `@requirements.txt`:
- Line 5: Update the Pillow dependency constraint in requirements.txt to require
the first release that patches all reported advisories, rather than allowing
11.3.0; preserve the existing native AVIF support requirement and use the
verified firstPatchedVersion for the minimum.
---
Nitpick comments:
In `@core/config.py`:
- Around line 138-149: Update Settings.save() to catch filesystem OSError
failures from writing or replacing the temporary config file, preserving the
callback without an unhandled exception. Narrow Settings.load() to catch only
the JSON parsing and file-read errors needed for corrupt or unavailable config
tolerance, allowing TypeError from cls.from_dict() to propagate instead of
silently resetting settings.
- Around line 34-53: Replace the import-time CONFIG_FILE initialization with a
config_file() function that resolves the current config_dir() lazily and creates
directories only when settings are accessed. Update the settings save() and
load() methods to call config_file() for their target path, preserving existing
read/write behavior and allowing WEBP_STUDIO_CONFIG_DIR changes after import.
In `@core/imaging.py`:
- Around line 17-19: Replace the global Image.MAX_IMAGE_PIXELS = None assignment
with a finite high ceiling that accommodates the supported 100 MP scans while
retaining Pillow’s decompression-bomb protection. Keep the change scoped to the
imaging initialization near Image.MAX_IMAGE_PIXELS.
In `@core/runner.py`:
- Around line 175-179: Copy the incoming Settings object in the runner’s
__init__ instead of retaining the GUI-owned instance, then use that snapshot
through run(), _claim_destination, and convert_file. Preserve the existing clamp
behavior on the runner-owned copy so worker reads remain consistent throughout
the batch.
- Around line 244-249: Refactor the claim loop around the lock in the runner
method so the lock only protects reserving and recording the candidate name, not
candidate.exists() filesystem probes. Reserve the lowercased name under
self._lock, check existence outside the lock, and retry when the reservation
loses a race; preserve _record’s locking and existing collision/index behavior.
In `@gui/panel.py`:
- Around line 218-225: Update _set_resize_mode to normalize resize_value when
switching modes, including replacing small values such as 100 with the valid
pixel-mode default rather than allowing them through clamp(). Make
_reload_resize_entry and _changed consistent with _commit_resize_value, removing
the redundant reload while preserving the visible-row synchronization.
In `@gui/screens/home.py`:
- Around line 265-269: Rename SettingsPanel’s _reload_controls method to the
public reload_controls method and update on_show to call reload_controls instead
of the private member. Preserve a thin _reload_controls alias only where
existing internal callers still require it.
In `@gui/screens/progress.py`:
- Around line 120-144: The unbounded LogView.append loops should be capped
before rendering. In gui/screens/progress.py#L120-L144, collect drained
file-event lines, then append only the last LogView.MAX_LINES entries after the
drain loop; in gui/screens/results.py#L137-L156, combine failed, skipped, and
note lines into one list, truncate it to LogView.MAX_LINES, and append the
truncated list.
In `@gui/screens/results.py`:
- Around line 159-169: Update open_output to use os.startfile(str(folder)) on
Windows instead of invoking explorer via subprocess. Preserve the existing early
return and platform-specific behavior for macOS and other systems, and keep
subprocess executable resolution unchanged outside Windows.
In `@gui/widgets.py`:
- Around line 143-146: Remove the redundant int() wrapper in _changed and assign
the rounded raw value directly, preserving the existing label update and
_on_change behavior.
In `@tests/test_engine.py`:
- Around line 92-109: Add a test_strip_gps test alongside test_metadata that
skips when PIEXIF_OK is false, creates an image containing both GPS and non-GPS
EXIF, and converts it with Settings(keep_metadata=True, strip_gps=True). Verify
the output retains EXIF and the Make field while its GPS IFD is empty, covering
the piexif-backed privacy path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69dd3d0c-628d-49e1-af91-387bbcf094bd
📒 Files selected for processing (27)
.github/workflows/build.yml.github/workflows/build_mac_app.yml.gitignoreREADME.mdcore/__init__.pycore/config.pycore/converter.pycore/imaging.pycore/runner.pygui/__init__.pygui/app.pygui/components/__init__.pygui/panel.pygui/screens/__init__.pygui/screens/dropzone.pygui/screens/home.pygui/screens/progress.pygui/screens/results.pygui/screens/settings.pygui/theme.pygui/widgets.pymain.pyrequirements.txtsetup.pytests/__init__.pytests/test_engine.pytests/test_gui_boot.py
💤 Files with no reviewable changes (5)
- gui/components/init.py
- .github/workflows/build_mac_app.yml
- gui/screens/dropzone.py
- gui/screens/settings.py
- core/converter.py
tkinterdnd2 grafts drop_target_register/dnd_bind onto tkinter.BaseWidget, but tkinter.Tk is not a BaseWidget subclass — so the root window never got those methods and every registration raised. The bare `except` around the setup swallowed it and showed "drag & drop unavailable on this build", which reads exactly like the package simply not being installed. It was installed. Mixing TkinterDnD.DnDWrapper into the window class fixes it, and the handler now prints the reason to stderr instead of hiding it. Caught by installing the optional dependency and checking app.dnd_enabled rather than trusting the tagline. test_gui_boot now simulates a real drop, using the brace-quoted string tkdnd actually delivers so a path containing spaces is covered, and asserts that a drop arriving mid-run is ignored rather than silently discarded. Also: the Intel bundle job targeted macos-13, which GitHub has retired. An unknown runner label queues indefinitely instead of failing, so that job hung while the other four passed. Switched to macos-15-intel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Fourteen findings, all valid; each verified against the code before changing anything. Two were already fixed in 9b122d2. Data integrity - The overwrite policy destroyed a good previous output when the new encode failed: the failure handler unlinked the destination unconditionally. Encoding now goes to a .part sidecar and is renamed over the target only on success, so a failed run leaves the previous output untouched. Also makes every write atomic — a half-written file never appears at the destination. Stability - An exception escaping _process aborted the entire batch and skipped on_finish, stranding the user on the progress screen with no completion and no results. The likely trigger was the unlink above raising PermissionError on Windows when an antivirus or indexer held the handle. _process can no longer raise, staging cleanup tolerates OSError, and run() fires on_finish from a finally block. - Closing the window during a run cancelled the runner and immediately destroyed the interpreter underneath a worker mid-write. Now joins the batch thread with a 5s timeout first. - save_log let an OSError escape into a console the user cannot see. Reports the failure in the details pane instead. Correctness - Presets could select an output format the installed Pillow cannot encode, the exact failure the capability probe exists to prevent. clamp() now reconciles output_format against writable_formats() (lazy-imported and cached; core.imaging imports core.config). - WebP's 16383px ceiling was hit at encode time with a message from inside the encoder. Checked up front with one that names the limit and the way out. - The lossless toggle was offered for AVIF, where quality=100 is near-lossless, not lossless. It is now WebP-only: JPEG has no lossless mode and PNG is always lossless, so the switch was noise in both. - The quality clamp fell back to 82 while the field default was 85. Performance - Every settings change triggered a full background rescan, so dragging the quality slider spawned one directory walk per pixel of travel — punishing on a large or network folder. Only the destination settings change which files are found, so only those trigger a rescan. Measured: 36 slider ticks now cause 0 scans, changing the destination causes exactly 1. Interface - A drop where every path was missing (ejected volume, dead network share) was silently ignored and looked like a broken app. It now says so. Security - Pillow floor raised to 12.3.0. Verified against the GitHub advisory database rather than taken on trust: everything below 12.3.0 carries HIGH advisories including a heap out-of-bounds write in ImageCmsTransform.apply (GHSA-9hw9-ch79-4vh6) and another in crop/paste (GHSA-6r8x-57c9-28j4). This app calls ImageCms on profiled images and crop/paste in the square modes. - Workflow declares permissions: contents: read, and the three checkout steps set persist-credentials: false. No job needs write access. Docs - README: macos-13 -> macos-15-intel, CustomTkinter version reconciled with requirements.txt, and the verification table now reflects that CI has run macOS successfully rather than claiming macOS is untested. Tests: three new engine checks cover the overwrite-preserves-output guarantee, the WebP dimension limit, and that every preset selects a writable format. 15 engine checks and the GUI boot suite pass on Windows with Pillow 12.3.0. Two findings needed no change: the macos-13 runner label and the inaccurate pool.map comment were both fixed in 9b122d2, before this review was posted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
All 14 findings addressed in f11add6. Each was checked against the code before changing anything; all were valid, and two had already been fixed in 9b122d2 before the review posted. Data integrity
StabilityAn exception escaping Closing the window mid-run cancelled the runner and destroyed the interpreter underneath a worker mid-write — now joins the batch thread with a 5s timeout.
Correctness
PerformanceEvery settings change triggered a full background rescan, so dragging the quality slider spawned one directory walk per pixel of travel. Only the destination settings change which files are found, so only those trigger a rescan. Measured after the fix: 36 slider ticks → 0 scans; changing the destination → exactly 1. Security
Already fixed before the review
TestsThree new engine checks: |
1 participant
Rewrite of MacAlpha v0.1. Same idea — drop images, get WebP — but the features the old README advertised now actually run.
Bugs fixed (all were live in v0.1)
tkinterdnd2was inrequirements.txtbut never imported; the drop zone was a clickable rectangle.gui/screens/dropzone.py:144piexifwas imported and unused; metadata was dropped from every output.core/converter.py:254run_batch_conversion()(thread pool) existed but was never called — the progress screen used a serialforloop, soMAX_THREADS: 8did nothing.gui/screens/progress.py:246start_timeassigned and never read. Results always showedTime: --.gui/screens/progress.py:244gui/screens/progress.py:247gui/screens/results.pygui/screens/progress.py:173iconfile: 'assets/icon.icns'was never committed.setup.py:19cd'd intoConvertImagesToWebP-MacAlpha/, a folder not in this repo..github/workflows/build_mac_app.yml:24Two more found while testing this branch, which also affected the new code until fixed:
Returnstarted a conversion while you were typing in a settings field.focus_get()returns the innertkinter.Entry, notCTkEntry, so the guard never matched."monospace"family; it silently fell back, so columns never aligned.New
Structure
core/has no UI imports, so the engine is scriptable and testable without a display.gui/theme.pyholds design tokens as(light, dark)pairs instead of greys hardcoded across fourteen call sites.Behaviour worth knowing:
Tests
tests/test_engine.py— 12 checks, no display neededtests/test_gui_boot.py— builds the real window and drives a real conversion through the Tk event loopCI runs the engine on Linux, the GUI on macOS + Windows, and builds Intel and Apple Silicon bundles separately. py2app bundles the interpreter it runs with, so a single runner produces a single-arch app that won't launch on the other kind of Mac — the old workflow would have shipped that silently.
Verification status
Merging this runs the macOS jobs for the first time. Note the built
.appis unsigned, so Gatekeeper will say "damaged and can't be opened" untilxattr -dr com.apple.quarantineis run — documented in the README.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests