minor bug fixes#22
Conversation
WalkthroughAdds undo handling for "auto_levels", swaps clockwise/counter-clockwise rotation direction, improves JPEG downsampling quality, tightens resource handling and typing, small UI/theme null-safety tweaks, updates SidecarManager constructor usage, and adjusts tests and cleanup logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI (QML)
participant App as AppController
participant FS as Filesystem / Image Storage
participant Cache as ByteLRUCache
participant Prefetch as Prefetcher
UI->>App: user triggers Undo (auto_levels)
App->>FS: check for backup image path
alt backup exists
FS-->>App: backup bytes
App->>App: restore image bytes into workspace
App->>Cache: clear display caches
App->>Cache: repopulate caches with restored image
App->>App: update current_index, display_generation
App->>Prefetch: cancel outstanding prefetch tasks
App->>Prefetch: enqueue prefetch for restored image
App->>UI: sync_ui_state(), update histogram (if needed)
App->>UI: set status "restore complete"
else backup missing
App->>App: log warning
App->>UI: set status "restore failed"
App->>App: re-add undo record for potential retry
end
Prefetch-->>Cache: deliver prefetched decoded images (async)
Cache-->>UI: responses for display requests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
faststack/faststack/ui/provider.py (1)
680-711: Crop box normalization + length guard look solid; optional extra validationThe normalization plus the “4‑element tuple only” guard is a good defensive boundary between QML and Python and should prevent most bad inputs from propagating.
If you ever see weird crop values coming from QML or plugins, you might optionally harden this further by validating element types (and possibly ranges) before accepting:
- # only accept 4-element tuples - if not isinstance(new_value, tuple) or len(new_value) != 4: - log.warning("UIState.currentCropBox: ignoring invalid crop box %r", new_value) - return + # only accept 4-element tuples of numeric values + if ( + not isinstance(new_value, tuple) + or len(new_value) != 4 + or not all(isinstance(v, (int, float)) for v in new_value) + ): + log.warning("UIState.currentCropBox: ignoring invalid crop box %r", new_value) + returnPurely optional; current behavior is already correct and safe.
faststack/faststack/tests/test_new_features.py (1)
39-46: Auto-levels assertions are effectively tautological now
b_scaledandw_scaledare defined asblacks * strength/whites * strength, and the assertions then compare them against the exact same expressions. Aside from catching NaN/inf, these checks don’t validate any real behavior ofauto_levelsor the strength scaling.Given the comments about “meaningful values” and expected behavior, it would be more useful for this test to assert properties of
blacks/whitesor their scaled values (e.g., ranges, ordering, or non-triviality) rather than re-checking an identity.Consider tightening these assertions so they fail if
auto_levelsregresses but still allow some numerical slack.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
faststack/faststack/app.py(1 hunks)faststack/faststack/imaging/cache.py(3 hunks)faststack/faststack/imaging/editor.py(1 hunks)faststack/faststack/imaging/jpeg.py(1 hunks)faststack/faststack/imaging/metadata.py(1 hunks)faststack/faststack/imaging/prefetch.py(0 hunks)faststack/faststack/qml/Components.qml(2 hunks)faststack/faststack/qml/ExifDialog.qml(1 hunks)faststack/faststack/tests/test_new_features.py(1 hunks)faststack/faststack/tests/test_sidecar.py(3 hunks)faststack/faststack/ui/provider.py(1 hunks)faststack/faststack/verify_wb.py(1 hunks)
💤 Files with no reviewable changes (1)
- faststack/faststack/imaging/prefetch.py
🧰 Additional context used
📓 Path-based instructions (5)
faststack/faststack/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
faststack/faststack/**/*.py: Use four-space indentation in Python files
Use trailing commas for multi-line structures in Python
Use descriptive snake_case identifiers for variables and functions in Python
Use CamelCase exclusively for class names and Qt signal names in Python
Include type hints in all Python code as seen inmodels.pyandAppController
Every new Python module should expose a top-levellog = logging.getLogger(__name__)logger
Format Python code with Black before opening a pull request
Lint Python code with Ruff before opening a pull request
Prefer relative paths in configuration and file handling
Files:
faststack/faststack/tests/test_sidecar.pyfaststack/faststack/tests/test_new_features.pyfaststack/faststack/imaging/cache.pyfaststack/faststack/ui/provider.pyfaststack/faststack/verify_wb.pyfaststack/faststack/imaging/editor.pyfaststack/faststack/app.pyfaststack/faststack/imaging/jpeg.pyfaststack/faststack/imaging/metadata.py
faststack/faststack/**/test_*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
faststack/faststack/**/test_*.py: Place new test cases intest_*.pyfiles located beside the modules being tested
Mock filesystem and Qt dependencies in tests to keep them deterministic
Mark tests that perform long disk scans withpytest.mark.slow
Files:
faststack/faststack/tests/test_sidecar.pyfaststack/faststack/tests/test_new_features.py
faststack/faststack/imaging/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
Cover every new branch in
imaging/orio/modules with focusedpytesttest cases
Files:
faststack/faststack/imaging/cache.pyfaststack/faststack/imaging/editor.pyfaststack/faststack/imaging/jpeg.pyfaststack/faststack/imaging/metadata.py
faststack/faststack/ui/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
Treat drag-and-drop helpers like
make_hdropas security-sensitive and validate all user-supplied paths before invoking external tools
Files:
faststack/faststack/ui/provider.py
faststack/faststack/**/app.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
app.pyis the PySide6 entrypoint that coordinates image indexing, caching, and QML screens
Files:
faststack/faststack/app.py
🧠 Learnings (9)
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/**/benchmark_*.py : When adding performance heuristics, pair the change with an updated benchmark script or validation notes
Applied to files:
faststack/faststack/tests/test_new_features.py
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/**/test_*.py : Mock filesystem and Qt dependencies in tests to keep them deterministic
Applied to files:
faststack/faststack/verify_wb.py
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/**/*.py : Prefer relative paths in configuration and file handling
Applied to files:
faststack/faststack/verify_wb.py
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/**/*.py : Use descriptive snake_case identifiers for variables and functions in Python
Applied to files:
faststack/faststack/verify_wb.py
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/**/*.py : Format Python code with Black before opening a pull request
Applied to files:
faststack/faststack/verify_wb.py
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/**/test_*.py : Place new test cases in `test_*.py` files located beside the modules being tested
Applied to files:
faststack/faststack/verify_wb.py
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/io/**/*.py : Cover every new branch in `imaging/` or `io/` modules with focused `pytest` test cases
Applied to files:
faststack/faststack/verify_wb.py
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/io/**/*.py : Validate all user-supplied paths before invoking external tools such as Helicon Focus
Applied to files:
faststack/faststack/verify_wb.py
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/**/app.py : `app.py` is the PySide6 entrypoint that coordinates image indexing, caching, and QML screens
Applied to files:
faststack/faststack/app.py
🧬 Code graph analysis (2)
faststack/faststack/tests/test_new_features.py (1)
faststack/faststack/ui/provider.py (4)
blacks(795-796)blacks(799-802)whites(805-806)whites(809-812)
faststack/faststack/imaging/editor.py (2)
faststack/faststack/config.py (1)
get(84-85)faststack/faststack/app.py (1)
rotate_image_ccw(2281-2287)
🔇 Additional comments (11)
faststack/faststack/imaging/jpeg.py (1)
167-173: LANCZOS fallback change looks correct; confirm performance on real workloadsThe switch to
Image.Resampling.LANCZOSforscale_factor_ratio <= 4aligns with the comment and should improve quality for moderate downscales/upsamples, while still using faster BILINEAR for very large downscales. Please sanity‑check that this doesn’t introduce unacceptable latency in your typical Pillow‑fallback paths (e.g., large images resized only modestly).faststack/faststack/qml/ExifDialog.qml (1)
28-41: Defensivecontrollerguards on open/close are appropriateWrapping
controller.dialog_opened()/controller.dialog_closed()inif (controller) { ... }is a good defensive fix and avoids runtime errors when the dialog is used in contexts wherecontrollerisn’t bound, without changing normal behavior when it is present.faststack/faststack/qml/Components.qml (1)
814-819: Theme-aware text colors in aspect ratio panel look goodSwitching these Text colors to
aspectRatioWindow.isDark ? "white" : "black"makes the aspect ratio header, entries, and “Rotate” label readable in both dark and light themes and is consistent with the panel’s background/border theming. No functional regressions visible.Also applies to: 830-837, 860-867
faststack/faststack/imaging/metadata.py (2)
56-58: LGTM: Appropriate use of broad exception handling.The broad exception catch with the
noqa: BLE001comment is reasonable here. EXIF parsing can fail in unpredictable ways (corrupted data, unsupported formats, malformed tags), and the function has a safe fallback behavior (returning empty dictionaries and logging a warning). This is good defensive programming.
52-53: The review comment is factually incorrect._getexif()was not removed in Pillow 10.0.0. The method remains functional and continues to work as expected with Pillow 10.x. Whilegetexif()was introduced as a public API in Pillow 6.0/8.2.0 as an alternative,_getexif()was never deprecated or removed. The code is currently working in production with Pillow 10.x, and the diff suggestion is unnecessary.Likely an incorrect or invalid review comment.
faststack/faststack/imaging/cache.py (2)
5-5: LGTM: Type hint improvements enhance code clarity.The addition of
Optionalto the typing imports and the explicit typing of theon_evictparameter improve type safety and align with the coding guidelines' requirement for comprehensive type hints.Also applies to: 19-19
7-7: > Likely an incorrect or invalid review comment.faststack/faststack/verify_wb.py (1)
59-63: Consolidated cleanup loop looks goodThe shared
for path in [black_path, grey_path]cleanup withOSErrorhandling keeps behavior the same while reducing duplication and avoiding failures if files are already gone or locked. Suits this helper script well.faststack/faststack/tests/test_sidecar.py (1)
23-24: SidecarManager constructor updates correctly track the new signatureAdapting all test call sites to
SidecarManager(d, None)matches the updated__init__(directory, extra=None)API without changing test semantics. This keeps the tests aligned with the production code while preserving existing expectations.Also applies to: 39-40, 50-51, 70-71
faststack/faststack/app.py (1)
1767-1806: auto_levels undo branch is consistent with existing undo flowsThe new
"auto_levels"branch correctly mirrors the backup-restore pattern used for auto white balance and crop:
- Restores the backup (unlink + rename) only if it exists, with robust
OSErrorhandling.- Refreshes the image list, repositions
current_index, bumpsdisplay_generation, clears cache, and restarts prefetching before syncing UI.- Additionally refreshes the histogram when visible, which matches how auto-level operations affect the displayed image.
This neatly closes the loop for
quick_auto_levelsactions recorded inundo_history.faststack/faststack/imaging/editor.py (1)
467-470: The rotation direction change is correct and consistent with the existing app.py implementation.The new code in
rotate_image_cw()(adding 90) androtate_image_ccw()(subtracting 90) matches the existing, documented behavior inapp.py(lines 2274-2287). This PR bringseditor.pyinto alignment with the controller logic. There is no rotation direction bug here—this is a consistency fix as indicated by the "minor bug fixes" commit message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
faststack/faststack/ui/provider.py (1)
705-710: Good defensive validation oncurrentCropBox; optional bounds clampThe new checks correctly reject non-tuples, wrong lengths, or non‑numeric elements and keep the last valid crop box, which is safe for
execute_cropand other consumers.If you want to harden further, you could also clamp each value into the expected 0–1000 range here so downstream code never sees out‑of‑range coordinates, but that’s optional.
faststack/faststack/app.py (1)
1767-1806:undo_auto_levelscorrectly mirrors other undo flows and refreshes UI/cacheThe new
"auto_levels"undo branch is wired cleanly intoundo_deleteand follows the same pattern as the AWB and crop undo branches:
- Restores the backup atomically with
unlink()+rename().- Rescans images, relocates
current_indexto the restored file, bumpsdisplay_generation, clears cache, cancels/restarts prefetch, and callssync_ui_state().- On success, also refreshes the histogram when visible and updates the status message.
- On missing backup or
OSError, logs, shows status, and re‑adds the undo record so the operation remains retryable.This fits the existing undo model and looks sound. One optional improvement would be to apply the same histogram refresh in the
"auto_white_balance"branch for symmetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
faststack/faststack/app.py(2 hunks)faststack/faststack/imaging/editor.py(1 hunks)faststack/faststack/tests/test_new_features.py(1 hunks)faststack/faststack/ui/provider.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/faststack/tests/test_new_features.py
🧰 Additional context used
📓 Path-based instructions (4)
faststack/faststack/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
faststack/faststack/**/*.py: Use four-space indentation in Python files
Use trailing commas for multi-line structures in Python
Use descriptive snake_case identifiers for variables and functions in Python
Use CamelCase exclusively for class names and Qt signal names in Python
Include type hints in all Python code as seen inmodels.pyandAppController
Every new Python module should expose a top-levellog = logging.getLogger(__name__)logger
Format Python code with Black before opening a pull request
Lint Python code with Ruff before opening a pull request
Prefer relative paths in configuration and file handling
Files:
faststack/faststack/app.pyfaststack/faststack/imaging/editor.pyfaststack/faststack/ui/provider.py
faststack/faststack/**/app.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
app.pyis the PySide6 entrypoint that coordinates image indexing, caching, and QML screens
Files:
faststack/faststack/app.py
faststack/faststack/imaging/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
Cover every new branch in
imaging/orio/modules with focusedpytesttest cases
Files:
faststack/faststack/imaging/editor.py
faststack/faststack/ui/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
Treat drag-and-drop helpers like
make_hdropas security-sensitive and validate all user-supplied paths before invoking external tools
Files:
faststack/faststack/ui/provider.py
🧠 Learnings (2)
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/**/app.py : `app.py` is the PySide6 entrypoint that coordinates image indexing, caching, and QML screens
Applied to files:
faststack/faststack/app.py
📚 Learning: 2025-12-08T04:46:41.603Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.603Z
Learning: Applies to faststack/faststack/ui/**/*.py : Treat drag-and-drop helpers like `make_hdrop` as security-sensitive and validate all user-supplied paths before invoking external tools
Applied to files:
faststack/faststack/ui/provider.py
🔇 Additional comments (2)
faststack/faststack/imaging/editor.py (1)
467-475: Rotation helpers and docstrings are now internally consistentThe docstrings for
rotate_image_cwandrotate_image_ccwcorrectly describe the existing(current -/+ 90) % 360behavior and are consistent with the controller’s rotation methods and_apply_editslogic. No further changes needed.faststack/faststack/app.py (1)
2273-2285: Rotation direction in controller now matches editor semantics
rotate_image_cwnow decrements rotation by 90° androtate_image_ccwincrements by 90°, modulo 360, matchingImageEditor.rotate_image_cw/ccwand the way_apply_editsinterprets therotationfield. This resolves the previous clockwise/counter‑clockwise inversion between controller and editor without changing the public API.
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.