Version 0.1#1
Conversation
WalkthroughIntroduces FastStack, a complete image viewer application for photographers. The PR adds configuration management, JPEG/RAW file indexing and pairing, high-performance image decoding with TurboJPEG fallback, background prefetching, filesystem watching, metadata handling via sidecar JSON, Helicon Focus integration, Qt/QML UI components, keyboard input handling, application state management, and comprehensive test coverage. Changes
Sequence DiagramsequenceDiagram
participant User
participant main
participant AppController
participant Backend as Backend<br/>(Cache, Prefetch, etc.)
participant UI as UI<br/>(QML, UIState)
participant FileSystem as FileSystem<br/>(Watcher, Sidecar)
User->>main: faststack /path/to/images
main->>AppController: Initialize (image_dir, engine)
AppController->>Backend: Set up Cache, Prefetcher, Watcher
AppController->>FileSystem: Load Sidecar metadata
AppController->>Backend: Load and index images
AppController->>UI: Wire UIState, register image provider
Backend->>Backend: Prefetch adjacent images (async)
AppController->>User: Show application window
User->>UI: Press arrow key / click menu
UI->>AppController: next_image() or toggle_flag()
AppController->>Backend: Get decoded image from cache
alt Cache hit
Backend->>UI: Return cached DecodedImage
else Cache miss
Backend->>Backend: Synchronously decode via Prefetcher
Backend->>UI: Return decoded image
end
AppController->>UI: Update UIState (index, metadata, image source)
UI->>User: Render image and metadata
Backend->>Backend: Update prefetch window (async)
User->>UI: Begin/end stack or request Helicon Focus
AppController->>FileSystem: Save stack info and metadata to sidecar
AppController->>FileSystem: Launch Helicon Focus with aggregated RAW files
User->>UI: Exit or close window
AppController->>AppController: Sync UI state, cancel prefetch tasks
AppController->>FileSystem: Save last index and metadata to sidecar
AppController->>Backend: Shutdown cache, watcher, prefetcher
rect rgba(0, 200, 150, 0.2)
Note over AppController: Graceful cleanup
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
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: 15
🧹 Nitpick comments (10)
faststack/requirements.txt (1)
1-9: Consider separating development and runtime dependencies.The requirements file mixes runtime dependencies (PySide6, PyTurboJPEG, numpy, etc.) with build and testing tools (pyinstaller, pytest). This can lead to bloated production installations.
Consider one of these approaches:
Option 1: Use pyproject.toml optional dependencies
Since you already havepyproject.toml, move these to optional dependency groups:In
pyproject.toml:[project.optional-dependencies] dev = [ "pytest>=8.0,<9.0", "pyinstaller>=6.0,<7.0", ]Then install with:
pip install -e ".[dev]"Option 2: Separate requirements files
Createrequirements-dev.txt:pytest==8.* pyinstaller==6.*And keep only runtime dependencies in
requirements.txt.faststack/faststack/ui/keystrokes.py (1)
39-39: Lower log level for key press events.Logging every key press at INFO level may create excessive log noise during normal operation.
-log.info(f"Key pressed: {event.key()}") +log.debug(f"Key pressed: {event.key()}")faststack/pyproject.toml (1)
28-28: Move pytest to optional dev dependencies.pytest is a testing framework and shouldn't be a runtime dependency.
dependencies = [ "PySide6>=6.0,<7.0", "PyTurboJPEG>=1.8,<2.0", "numpy>=2.0,<3.0", "cachetools>=5.0,<6.0", "watchdog>=4.0,<5.0", "typer>=0.12,<1.0", "Pillow>=10.0,<11.0", - "pytest>=8.0,<9.0", ] + +[project.optional-dependencies] +dev = [ + "pytest>=8.0,<9.0", +]faststack/faststack/io/sidecar.py (1)
49-52: Uselogging.exceptionfor better error diagnostics.When logging errors in exception handlers,
logging.exceptionautomatically includes the traceback, which aids debugging.except (json.JSONDecodeError, TypeError) as e: - log.error(f"Failed to load or parse sidecar file {self.path}: {e}") + log.exception(f"Failed to load or parse sidecar file {self.path}") # Consider backing up the corrupted file here return Sidecar()Note: The comment on line 51 suggests backing up corrupted files—consider implementing this to prevent data loss.
faststack/faststack/tests/test_pairing.py (1)
55-55: Consider splitting compound statements for PEP 8 compliance.Multiple statements are combined on single lines using semicolons, which reduces readability.
Example for Line 55:
- jpg_stat = MagicMock(); jpg_stat.st_mtime = 1000.0 + jpg_stat = MagicMock() + jpg_stat.st_mtime = 1000.0Apply similar changes to Lines 58, 63, 68, and 69.
Also applies to: 58-58, 63-63, 68-69
faststack/faststack/io/helicon.py (2)
50-52: Uselogging.exceptionfor better debugging.When logging in an exception handler,
logging.exceptionautomatically includes the stack trace.Apply this diff:
except Exception as e: - log.error(f"Failed to launch Helicon Focus: {e}") + log.exception("Failed to launch Helicon Focus") return False, None
46-47: Remove duplicate log statement.Lines 46 and 47 both log the command, with Line 46 at INFO level and Line 47 at DEBUG level. One is sufficient.
Apply this diff:
log.info(f"Launching Helicon Focus with {len(raw_files)} files.") - log.info(f"Helicon Focus command: {args}") # Log the full command log.debug(f"Command: {args}")faststack/faststack/qml/Components.qml (2)
72-72: Simplify redundant ternary operators.The outer condition already checks
uiState.isFlagged/uiState.isRejected, so the inner ternary is redundant.Apply this diff:
Text { - text: uiState && uiState.isFlagged ? `[${uiState.isFlagged ? 'F' : ''}]` : "" + text: uiState && uiState.isFlagged ? "[F]" : "" color: "lightgreen" font.bold: true } Text { - text: uiState && uiState.isRejected ? `[${uiState.isRejected ? 'X' : ''}]` : "" + text: uiState && uiState.isRejected ? "[X]" : "" color: "red" font.bold: true }Also applies to: 77-77
43-48: Consider adding zoom bounds.The current zoom implementation multiplies scale indefinitely without limits, which could lead to extreme zoom levels (either very large or very small).
Consider adding bounds:
onWheel: { var scaleFactor = wheel.angleDelta.y > 0 ? 1.2 : 1 / 1.2; var newScale = mainImage.scale * scaleFactor; // Clamp between 0.1x and 10x mainImage.scale = Math.max(0.1, Math.min(10.0, newScale)); }faststack/faststack/imaging/cache.py (1)
35-38: Consider moving import to module level.The import of
DecodedImageon Line 35 is inside the function. Per PEP 8, imports should typically be at the module level unless there's a specific reason (e.g., circular import).If there's no circular dependency, apply this diff:
import logging from typing import Any, Callable from cachetools import LRUCache +from faststack.models import DecodedImage log = logging.getLogger(__name__) ... def get_decoded_image_size(item) -> int: """Calculates the size of a decoded image tuple (buffer, qimage).""" # In this simplified example, we only store the buffer. # In the full app, this would also account for the QImage/QTexture. - from faststack.models import DecodedImage if isinstance(item, DecodedImage): return item.buffer.nbytes return 1 # Should not happen
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.gitignore(1 hunks)README.md(0 hunks)faststack/LICENSE(1 hunks)faststack/README.md(1 hunks)faststack/faststack/app.py(1 hunks)faststack/faststack/config.py(1 hunks)faststack/faststack/imaging/cache.py(1 hunks)faststack/faststack/imaging/jpeg.py(1 hunks)faststack/faststack/imaging/prefetch.py(1 hunks)faststack/faststack/io/helicon.py(1 hunks)faststack/faststack/io/indexer.py(1 hunks)faststack/faststack/io/sidecar.py(1 hunks)faststack/faststack/io/watcher.py(1 hunks)faststack/faststack/logging_setup.py(1 hunks)faststack/faststack/models.py(1 hunks)faststack/faststack/qml/Components.qml(1 hunks)faststack/faststack/qml/Main.qml(1 hunks)faststack/faststack/tests/test_cache.py(1 hunks)faststack/faststack/tests/test_pairing.py(1 hunks)faststack/faststack/tests/test_sidecar.py(1 hunks)faststack/faststack/ui/keystrokes.py(1 hunks)faststack/faststack/ui/provider.py(1 hunks)faststack/pyproject.toml(1 hunks)faststack/requirements.txt(1 hunks)faststack/test.py(1 hunks)
💤 Files with no reviewable changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (12)
faststack/faststack/imaging/cache.py (1)
faststack/faststack/models.py (1)
DecodedImage(30-39)
faststack/faststack/io/sidecar.py (3)
faststack/faststack/models.py (2)
Sidecar(22-27)EntryMetadata(15-18)faststack/faststack/app.py (1)
load(65-72)faststack/faststack/io/watcher.py (3)
stop(47-53)start(32-45)is_alive(55-57)
faststack/faststack/tests/test_cache.py (1)
faststack/faststack/imaging/cache.py (1)
ByteLRUCache(10-28)
faststack/faststack/config.py (1)
faststack/faststack/logging_setup.py (1)
get_app_data_dir(8-13)
faststack/faststack/io/helicon.py (1)
faststack/faststack/config.py (1)
get(57-58)
faststack/faststack/io/indexer.py (1)
faststack/faststack/models.py (1)
ImageFile(8-12)
faststack/faststack/imaging/prefetch.py (3)
faststack/faststack/models.py (2)
ImageFile(8-12)DecodedImage(30-39)faststack/faststack/imaging/jpeg.py (1)
decode_jpeg_rgb(22-39)faststack/faststack/app.py (1)
shutdown(205-216)
faststack/faststack/ui/keystrokes.py (1)
faststack/faststack/app.py (9)
next_image(105-109)prev_image(111-115)toggle_grid_view(117-118)toggle_current_flag(136-141)toggle_current_reject(143-148)begin_new_stack(150-153)end_current_stack(155-168)launch_helicon(170-188)clear_all_stacks(198-203)
faststack/faststack/tests/test_sidecar.py (2)
faststack/faststack/io/sidecar.py (4)
SidecarManager(12-90)set_last_index(89-90)get_metadata(85-87)save(54-83)faststack/faststack/models.py (1)
EntryMetadata(15-18)
faststack/faststack/app.py (11)
faststack/faststack/logging_setup.py (1)
setup_logging(15-35)faststack/faststack/models.py (3)
ImageFile(8-12)DecodedImage(30-39)EntryMetadata(15-18)faststack/faststack/io/indexer.py (1)
find_images(19-54)faststack/faststack/io/sidecar.py (5)
SidecarManager(12-90)load(26-52)get_metadata(85-87)save(54-83)set_last_index(89-90)faststack/faststack/io/watcher.py (3)
Watcher(24-57)start(32-45)stop(47-53)faststack/faststack/io/helicon.py (1)
launch_helicon_focus(13-52)faststack/faststack/imaging/cache.py (2)
ByteLRUCache(10-28)get_decoded_image_size(31-38)faststack/faststack/imaging/prefetch.py (5)
Prefetcher(13-108)update_prefetch(29-49)set_image_files(25-27)submit_task(51-60)shutdown(104-108)faststack/faststack/ui/provider.py (2)
ImageProvider(15-49)UIState(51-106)faststack/faststack/ui/keystrokes.py (2)
Keybinder(8-44)handle_key_press(37-44)faststack/faststack/config.py (3)
getint(60-61)load(28-44)save(47-55)
faststack/faststack/ui/provider.py (2)
faststack/faststack/models.py (1)
DecodedImage(30-39)faststack/faststack/app.py (5)
get_decoded_image(80-94)get_current_metadata(120-134)next_image(105-109)prev_image(111-115)toggle_current_flag(136-141)
faststack/faststack/tests/test_pairing.py (1)
faststack/faststack/io/indexer.py (2)
find_images(19-54)_find_raw_pair(56-80)
🪛 LanguageTool
faststack/README.md
[style] ~3-~3: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ... FastStack # Version 0.1 - October 31, 2025 # By Alan Rockefeller Ultra-fast, cach...
(MISSING_COMMA_AFTER_YEAR)
🪛 Ruff (0.14.2)
faststack/faststack/io/sidecar.py
50-50: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
80-80: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/config.py
55-55: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/io/helicon.py
48-48: subprocess call: check for execution of untrusted input
(S603)
49-49: Consider moving this statement to an else block
(TRY300)
50-50: Do not catch blind exception: Exception
(BLE001)
51-51: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/io/indexer.py
38-38: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/imaging/prefetch.py
87-87: Do not catch blind exception: Exception
(BLE001)
88-88: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/imaging/jpeg.py
28-28: Do not catch blind exception: Exception
(BLE001)
29-29: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
37-37: Do not catch blind exception: Exception
(BLE001)
38-38: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
55-55: Do not catch blind exception: Exception
(BLE001)
56-56: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
64-64: Do not catch blind exception: Exception
(BLE001)
65-65: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/io/watcher.py
27-27: Undefined name Optional
(F821)
faststack/faststack/tests/test_sidecar.py
14-14: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
faststack/faststack/app.py
176-176: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
196-196: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
231-231: Do not perform function call typer.Argument in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
faststack/faststack/ui/provider.py
22-22: Unused method argument: size
(ARG002)
22-22: Unused method argument: requestedSize
(ARG002)
47-47: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/tests/test_pairing.py
55-55: Multiple statements on one line (semicolon)
(E702)
58-58: Multiple statements on one line (semicolon)
(E702)
58-58: Multiple statements on one line (semicolon)
(E702)
63-63: Multiple statements on one line (semicolon)
(E702)
63-63: Multiple statements on one line (semicolon)
(E702)
68-68: Multiple statements on one line (semicolon)
(E702)
68-68: Multiple statements on one line (semicolon)
(E702)
69-69: Multiple statements on one line (semicolon)
(E702)
69-69: Multiple statements on one line (semicolon)
(E702)
🔇 Additional comments (14)
faststack/LICENSE (1)
1-25: MIT License is well-suited for this project.The license file is correctly formatted with the standard MIT License text and includes an appropriate copyright notice. MIT is a permissive open-source license that aligns well with a community-driven image viewer application.
.gitignore (1)
1-25: Well-structured.gitignorewith comprehensive Python patterns.The file is well-organized with clear sections covering virtual environments, Python cache, build outputs, logs, OS files, and IDE configurations. The patterns follow Python project best practices and the addition of
prompt.mdappropriately excludes project-specific documentation.faststack/faststack/io/watcher.py (1)
32-57: LGTM! Watcher lifecycle management is well-implemented.The implementation correctly:
- Validates directory existence before starting
- Prevents multiple concurrent observers
- Creates fresh Observer instances (avoiding restart issues)
- Properly stops and cleans up resources
faststack/faststack/io/sidecar.py (2)
54-83: Excellent atomic save implementation with minor logging improvement.The atomic save pattern (write to temp file, then rename) is robust and prevents corruption. Stopping the watcher during save prevents recursive events.
Minor improvement for consistency:
except (IOError, TypeError) as e: - log.error(f"Failed to save sidecar file {self.path}: {e}") + log.exception(f"Failed to save sidecar file {self.path}") finally:
85-90: LGTM! Clean metadata accessor methods.The use of
setdefaultfor get-or-create pattern inget_metadatais idiomatic and efficient.faststack/faststack/imaging/jpeg.py (3)
22-39: Robust fallback implementation with minor logging improvement.The two-tier fallback (TurboJPEG → Pillow → None) provides excellent resilience. Catching broad
Exceptionis justified here since any decode failure should trigger fallback.Consider using
logging.exceptionfor automatic traceback capture:except Exception as e: - log.error(f"PyTurboJPEG failed to decode image: {e}. Trying Pillow.") + log.exception("PyTurboJPEG failed to decode image. Trying Pillow.")except Exception as e: - log.error(f"Pillow also failed to decode image: {e}") + log.exception("Pillow also failed to decode image") return NoneOptional: Move
BytesIOimport to module level (line 4) for minor efficiency gain.
41-66: Well-implemented thumbnail decoding with scaling optimization.Leveraging TurboJPEG's native scaling (N/8 factors) for thumbnail generation is efficient.
Apply the same logging improvements:
except Exception as e: - log.error(f"PyTurboJPEG failed to decode thumbnail: {e}. Trying Pillow.") + log.exception("PyTurboJPEG failed to decode thumbnail. Trying Pillow.")except Exception as e: - log.error(f"Pillow also failed to decode thumbnail: {e}") + log.exception("Pillow also failed to decode thumbnail") return None
68-74: LGTM! Correct implementation of TurboJPEG scaling factor selection.The algorithm correctly iterates from largest to smallest scaling factor to find the best fit within
max_dim.faststack/faststack/models.py (1)
1-39: LGTM! Clean data model definitions.The dataclasses are well-structured with appropriate type hints and defaults. The
__sizeof__implementation onDecodedImagecorrectly returns the buffer size for memory accounting.faststack/faststack/tests/test_cache.py (1)
1-61: LGTM! Comprehensive cache test coverage.The tests thoroughly exercise the ByteLRUCache behavior, including initialization, size tracking, eviction policy, and item updates. The
MockItemapproach for testing size accounting is clean and effective.faststack/faststack/logging_setup.py (1)
1-35: LGTM! Well-configured logging setup.The logging configuration is appropriate for a desktop application with rotating file handler (10MB, 5 backups) and sensible defaults. The platform-specific app data directory logic correctly handles Windows and Unix-like systems.
faststack/faststack/tests/test_pairing.py (1)
36-51: LGTM! Test coverage is thorough.The
test_find_imagesfunction effectively validates the RAW-JPG pairing logic with realistic scenarios.faststack/faststack/imaging/cache.py (1)
10-28: LGTM! Well-implemented byte-aware cache.The
ByteLRUCacheproperly extendsLRUCachewith size-based eviction and informative logging. The implementation correctly delegates to the parent class for eviction logic.faststack/faststack/io/helicon.py (1)
33-49: No action required. The review comment is incorrect.Cleanup of the temporary file is properly implemented in the caller code (app.py lines 183-186). The function
launch_helicon_focusreturns thetmp_path, which the caller schedules for deletion viaQTimer.singleShot(5000, lambda: self._delete_temp_file(tmp_path)). The_delete_temp_filemethod (lines 190-195) then removes the file usingos.remove()with proper error handling. This is a valid architectural pattern where the caller takes ownership of the temporary file lifecycle.Likely an incorrect or invalid review comment.
1 participant
Version 0.1
Summary by CodeRabbit
New Features
Documentation