Skip to content

Handle missing turbojpeg DLL on Windows#53

Closed
Dj-Shortcut wants to merge 6 commits intoAlanRockefeller:mainfrom
Dj-Shortcut:mijn-volgende-branch
Closed

Handle missing turbojpeg DLL on Windows#53
Dj-Shortcut wants to merge 6 commits intoAlanRockefeller:mainfrom
Dj-Shortcut:mijn-volgende-branch

Conversation

@Dj-Shortcut
Copy link
Copy Markdown

@Dj-Shortcut Dj-Shortcut commented Mar 17, 2026

Fix Windows startup issues when turbojpeg.dll is missing and document the setup for Windows users.

This PR makes FastStack fall back cleanly when the native libjpeg-turbo library is not installed, instead of failing during startup. It also adds shared
TurboJPEG discovery logic, improves app-data directory fallback handling, and documents the Windows setup and troubleshooting steps in the README.

Summary by CodeRabbit

  • New Features

    • Optional libjpeg-turbo support with automatic discovery for faster JPEG decoding and thumbnails; falls back to Pillow when unavailable.
  • Documentation

    • Windows-specific setup guide: install steps, recommended locations, env vars, usage example, and troubleshooting.
  • Improvements

    • Smarter app-data directory selection with writability checks.
    • Console logging always enabled; file logging is now optional and disabled gracefully on errors.
    • Centralized decoder initialization for more reliable discovery.
  • Tests

    • Added unit tests for decoder discovery and logging/app-data behaviors.
  • Chores

    • Test-runner and ignore-file tweaks for local pytest artifacts.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 17, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Centralizes TurboJPEG discovery/initialization in a new imaging.turbo module, updates imaging and thumbnail code to use it (falling back to Pillow), refactors app-data directory selection and guarded file-logging setup, adds Windows README notes, test coverage, and pytest config/gitignore updates.

Changes

Cohort / File(s) Summary
TurboJPEG discovery module
faststack/imaging/turbo.py
New module that enumerates candidate native library paths (env vars, common Windows locations, PATH), deduplicates them, and exposes _candidate_library_paths() and create_turbojpeg() which attempts initialization per candidate and returns (decoder, available) with detailed warnings on failures.
Imaging integration
faststack/imaging/jpeg.py
Replaces inline TurboJPEG import/try-except with create_turbojpeg() import; uses returned (decoder, availability) and TJPF_RGB for decode paths, preserving Pillow fallback and existing resampling logic.
Thumbnail prefetcher
faststack/thumbnail_view/prefetcher.py
Switched from direct TurboJPEG import to create_turbojpeg() (module-level _tj, HAS_TURBOJPEG) and TJPF_RGB, retaining prior decode/thumbnail fallback behavior.
Logging & app-data handling
faststack/logging_setup.py
Adds _is_writable_directory() and _can_create_directory() helpers; refactors get_app_data_dir() to prefer multiple candidate bases (including FASTSTACK_APPDATA, APPDATA, LOCALAPPDATA, HOME, temp, cwd) and return the first writable/createable path; setup_logging() now always adds a console handler and only installs a rotating file handler inside a guarded try/except.
Documentation
README.md
Adds Windows-specific guidance for installing/locating libjpeg-turbo (turbojpeg.dll), environment variable overrides, usage examples, and troubleshooting when PyTurboJPEG is missing (Pillow fallback noted).
Tests & config
faststack/tests/test_turbo.py, pytest.ini, .gitignore
New unit tests for TurboJPEG discovery, app-data fallback, writability checks, and logging resiliency; adds pytest.ini to control basetemp and ignored dirs; extends .gitignore for pytest/temp dirs.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Consumer as Consumer (jpeg / prefetcher)
    participant TurboMod as faststack.imaging.turbo
    participant Env as OS/Env
    participant FS as Filesystem
    participant Native as native turbojpeg.dll
    participant Pillow as Pillow decoder

    Consumer->>TurboMod: call create_turbojpeg()
    TurboMod->>Env: read env vars (FASTSTACK_TURBOJPEG_LIB/ROOT, PATH, ProgramFiles...)
    TurboMod->>FS: build & probe candidate paths
    TurboMod->>Native: attempt TurboJPEG(candidate) init
    alt init succeeds
        Native-->>TurboMod: decoder instance
        TurboMod-->>Consumer: return (decoder, True)
        Consumer->>Native: perform fast decode/resize
    else all attempts fail or turbojpeg missing
        TurboMod-->>Consumer: return (None, False)
        Consumer->>Pillow: fallback decode/thumbnail via Pillow
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Handle missing turbojpeg DLL on Windows' accurately summarizes the primary change—adding graceful fallback when the native libjpeg-turbo library is absent on Windows, which is the main objective of this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
faststack/logging_setup.py (1)

9-19: Side effect: directories are created during writability probe.

The _is_writable_dir function creates directories (via mkdir(parents=True)) for every candidate it checks, not just the one ultimately selected. This leaves behind empty directories in locations like ~/.faststack or cwd/var/appdata even when a higher-priority candidate (e.g., APPDATA) is ultimately used.

Consider checking candidate.exists() and os.access(candidate, os.W_OK) first, and only create + probe if the directory doesn't exist or the quick check passes.

♻️ Suggested improvement to reduce side effects
 def _is_writable_dir(path: Path) -> bool:
     """Return True when the directory can be created and written to."""
     try:
+        if path.exists():
+            # Quick check before probing
+            if not path.is_dir():
+                return False
         path.mkdir(parents=True, exist_ok=True)
         probe = path / ".write_test"
         with probe.open("w", encoding="utf-8") as f:
             f.write("ok")
         probe.unlink(missing_ok=True)
         return True
     except OSError:
         return False
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@faststack/logging_setup.py` around lines 9 - 19, The helper _is_writable_dir
currently always creates the directory when probing, causing unwanted
side-effect directories; change it to first check if the Path exists and is
writable using Path.exists() and os.access(path, os.W_OK) (or path.stat +
permission checks) and ONLY if that quick check fails perform the safe
create-and-probe sequence (mkdir + write a temp file and cleanup). Keep the
function signature and return semantics the same and ensure you only call
path.mkdir(parents=True, exist_ok=True) when the directory does not exist or the
access check indicates writability cannot be determined.
faststack/imaging/turbo.py (1)

76-77: Consider logging a warning when all discovery attempts fail.

When create_turbojpeg() returns (None, False) after trying all candidates, only debug-level logs are emitted. Since this affects performance (Pillow fallback is slower), an info or warning log summarizing the failure would help users troubleshoot without enabling debug logging.

💡 Suggested addition before the final return
     for candidate in _candidate_library_paths():
         if not candidate.exists():
             continue
         try:
             decoder = TurboJPEG(lib_path=str(candidate))
             log.info("Using libjpeg-turbo at %s", candidate)
             return decoder, True
         except Exception:
             log.debug("Failed to initialize turbojpeg from %s", candidate)

+    log.info(
+        "TurboJPEG native library not found; JPEG decoding will use Pillow (slower). "
+        "See README for installation instructions."
+    )
     return None, False
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@faststack/imaging/turbo.py` around lines 76 - 77, The function
create_turbojpeg currently returns (None, False) silently after trying all
candidates; add a higher-severity log (logger.warning or logging.warning) just
before that final return to summarize that all turbojpeg discovery attempts
failed, include which candidates were tried and that Pillow fallback will be
used, and reference the create_turbojpeg function and the candidates variable to
locate where to insert the warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@faststack/logging_setup.py`:
- Around line 41-47: get_app_data_dir() currently returns the first writable
candidate or falls back to candidates[0] even if unwritable which can change
across runs; update get_app_data_dir() to first scan all candidates (using the
same _is_writable_dir check) for an existing faststack.ini (referencing
AppConfig and the filename faststack.ini) and return that candidate if found,
and if none exists then either return the first writable candidate as before or,
when falling back to an unwritable candidates[0], emit a warning via the logger
indicating the chosen fallback differs from higher-priority candidates so
callers know the path may be unstable.

---

Nitpick comments:
In `@faststack/imaging/turbo.py`:
- Around line 76-77: The function create_turbojpeg currently returns (None,
False) silently after trying all candidates; add a higher-severity log
(logger.warning or logging.warning) just before that final return to summarize
that all turbojpeg discovery attempts failed, include which candidates were
tried and that Pillow fallback will be used, and reference the create_turbojpeg
function and the candidates variable to locate where to insert the warning.

In `@faststack/logging_setup.py`:
- Around line 9-19: The helper _is_writable_dir currently always creates the
directory when probing, causing unwanted side-effect directories; change it to
first check if the Path exists and is writable using Path.exists() and
os.access(path, os.W_OK) (or path.stat + permission checks) and ONLY if that
quick check fails perform the safe create-and-probe sequence (mkdir + write a
temp file and cleanup). Keep the function signature and return semantics the
same and ensure you only call path.mkdir(parents=True, exist_ok=True) when the
directory does not exist or the access check indicates writability cannot be
determined.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0a44eb70-366c-460c-916a-48a72e3d784f

📥 Commits

Reviewing files that changed from the base of the PR and between c65442a and 8d583f3.

📒 Files selected for processing (5)
  • README.md
  • faststack/imaging/jpeg.py
  • faststack/imaging/turbo.py
  • faststack/logging_setup.py
  • faststack/thumbnail_view/prefetcher.py

Comment thread faststack/logging_setup.py Outdated
@Dj-Shortcut Dj-Shortcut force-pushed the mijn-volgende-branch branch from 8b35bc8 to 8d583f3 Compare March 17, 2026 18:34
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
faststack/thumbnail_view/model.py (1)

612-622: ⚠️ Potential issue | 🟠 Major

Same key format concern applies here.

This metadata_map.get(normalize_path_key(img.path), {}) lookup has the same potential mismatch with sidecar-generated keys as noted above.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@faststack/thumbnail_view/model.py` around lines 612 - 622, The metadata_map
lookup uses normalize_path_key(img.path) which may not match keys produced by
the sidecar generator; update the lookup in the block around metadata_map/get so
it uses the same key format as the sidecar (or try both forms). Specifically, in
the code that does metadata_map.get(normalize_path_key(img.path), {}), replace
or augment that call to use the canonical sidecar key (the same normalizer used
when writing sidecars) or attempt alternate keys (e.g., raw img.path, the
existing normalize_path_key, and the sidecar-normalized key) before falling back
to {} so metadata_map and self._get_metadata(img.path) resolve consistently.
Ensure you reference normalize_path_key, metadata_map, img.path and
_get_metadata when making the change.
🧹 Nitpick comments (2)
faststack/app.py (1)

2125-2155: De-duplicate the default metadata payload to prevent drift.

Line 2130-2137 and Line 2148-2155 duplicate the same fallback dict. Keeping one source avoids silent divergence when fields evolve.

♻️ Suggested cleanup
 def _get_metadata_dict(self, image_path: Path | str) -> dict:
     """Get metadata for an image path as a dict for thumbnail model."""
+    default_meta = {
+        "stacked": False,
+        "uploaded": False,
+        "edited": False,
+        "restacked": False,
+        "favorite": False,
+        "todo": False,
+    }
     try:
         meta = self.sidecar.get_metadata(image_path, create=False)
         if meta is None:
-            return {
-                "stacked": False,
-                "uploaded": False,
-                "edited": False,
-                "restacked": False,
-                "favorite": False,
-                "todo": False,
-            }
+            return default_meta.copy()
         return {
             "stacked": meta.stacked,
             "uploaded": meta.uploaded,
             "edited": meta.edited,
             "restacked": meta.restacked,
             "favorite": meta.favorite,
             "todo": meta.todo,
         }
     except Exception as e:  # Broad catch for UI plumbing - don't crash grid view
         log.debug("Failed to get metadata for %s: %s", image_path, e)
-        return {
-            "stacked": False,
-            "uploaded": False,
-            "edited": False,
-            "restacked": False,
-            "favorite": False,
-            "todo": False,
-        }
+        return default_meta.copy()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@faststack/app.py` around lines 2125 - 2155, The _get_metadata_dict function
duplicates the same default fallback dict in two places; extract a single
canonical default (e.g., DEFAULT_METADATA or a helper function like
_default_metadata_dict) and use it for both the None and exception return paths
to avoid drift, and when returning it ensure you return a fresh dict (copy) so
callers can't mutate the shared constant; update references in
_get_metadata_dict and any related code that constructs the metadata using
sidecar.get_metadata accordingly.
faststack/io/sidecar.py (1)

164-173: Avoid the full-entry scan on every miss.

When neither the stable key nor the candidate keys hit, this fallback walks the entire sidecar and normalizes every stored key. On large folders, a batch of new images turns get_metadata() misses into quadratic work. Consider building a one-time alias map during load() or gating this scan behind a “legacy keys present” flag so steady-state lookups stay O(1).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@faststack/io/sidecar.py` around lines 164 - 173, The code in get_metadata
(the block using self.data.entries and _stable_key_from_key) does a full scan on
misses; instead, add a one-time alias map or legacy-keys flag during load() to
make lookups O(1): populate self._alias_map (mapping stable_key ->
existing_key/meta) or set self._has_legacy_keys by iterating self.data.entries
once in load(), and then change get_metadata to consult self._alias_map or
short-circuit when self._has_legacy_keys is false; only if the alias_map misses
(or the flag indicates legacy keys) perform a targeted update/scan and update
alias_map accordingly. Ensure you update any code that currently mutates
self.data.entries to also update self._alias_map and the flag so steady-state
lookups avoid the full-entry scan.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@faststack/app.py`:
- Line 2230: The INFO logs currently emit full absolute file paths (image_path),
which can leak user directory info; change those log.info calls to log the
filename only by using os.path.basename(image_path) (e.g., replace
log.info("Toggled uploaded flag to %s for %s", meta.uploaded, image_path) with
log.info("Toggled uploaded flag to %s for %s", meta.uploaded,
os.path.basename(image_path))) and add a debug-level log that retains the full
path (e.g., log.debug("Full image path: %s", image_path)) so the detailed path
remains available for debugging; apply the same change to the other similar
occurrences that use image_path in INFO logs (the other log.info lines noted in
the comment).

In `@faststack/io/sidecar.py`:
- Around line 196-213: The code incorrectly treats strings with dots as paths;
in _lookup_keys and _stable_key_from_key remove the check that uses "." in
Path(value).name and instead only treat a value as path-like when it contains
path separators (os.path.sep, "/" or "\\") or when Path(value) resolves to an
existing file under self.directory; when path-like, call
metadata_key_for_path(path) and _metadata_filename_key(path) and use path.stem
as before, otherwise treat the value as a semantic key (return value and
[value]); ensure you add an existence check (Path(self.directory) /
value).exists() or similar to decide path inference so keys like "sample.v1" are
preserved.

---

Outside diff comments:
In `@faststack/thumbnail_view/model.py`:
- Around line 612-622: The metadata_map lookup uses normalize_path_key(img.path)
which may not match keys produced by the sidecar generator; update the lookup in
the block around metadata_map/get so it uses the same key format as the sidecar
(or try both forms). Specifically, in the code that does
metadata_map.get(normalize_path_key(img.path), {}), replace or augment that call
to use the canonical sidecar key (the same normalizer used when writing
sidecars) or attempt alternate keys (e.g., raw img.path, the existing
normalize_path_key, and the sidecar-normalized key) before falling back to {} so
metadata_map and self._get_metadata(img.path) resolve consistently. Ensure you
reference normalize_path_key, metadata_map, img.path and _get_metadata when
making the change.

---

Nitpick comments:
In `@faststack/app.py`:
- Around line 2125-2155: The _get_metadata_dict function duplicates the same
default fallback dict in two places; extract a single canonical default (e.g.,
DEFAULT_METADATA or a helper function like _default_metadata_dict) and use it
for both the None and exception return paths to avoid drift, and when returning
it ensure you return a fresh dict (copy) so callers can't mutate the shared
constant; update references in _get_metadata_dict and any related code that
constructs the metadata using sidecar.get_metadata accordingly.

In `@faststack/io/sidecar.py`:
- Around line 164-173: The code in get_metadata (the block using
self.data.entries and _stable_key_from_key) does a full scan on misses; instead,
add a one-time alias map or legacy-keys flag during load() to make lookups O(1):
populate self._alias_map (mapping stable_key -> existing_key/meta) or set
self._has_legacy_keys by iterating self.data.entries once in load(), and then
change get_metadata to consult self._alias_map or short-circuit when
self._has_legacy_keys is false; only if the alias_map misses (or the flag
indicates legacy keys) perform a targeted update/scan and update alias_map
accordingly. Ensure you update any code that currently mutates self.data.entries
to also update self._alias_map and the flag so steady-state lookups avoid the
full-entry scan.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cfe59918-0f33-4162-9382-49b1d0e3ff17

📥 Commits

Reviewing files that changed from the base of the PR and between 5184983 and 8b35bc8.

📒 Files selected for processing (9)
  • 8
  • faststack/app.py
  • faststack/io/sidecar.py
  • faststack/logging_setup.py
  • faststack/tests/test_jump_to_last_uploaded.py
  • faststack/tests/test_sidecar.py
  • faststack/tests/thumbnail_view/test_folder_stats.py
  • faststack/thumbnail_view/folder_stats.py
  • faststack/thumbnail_view/model.py

Comment thread faststack/app.py Outdated
status = "uploaded" if meta.uploaded else "not uploaded"
self.update_status_message(f"Marked as {status}")
log.info("Toggled uploaded flag to %s for %s", meta.uploaded, stem)
log.info("Toggled uploaded flag to %s for %s", meta.uploaded, image_path)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid full absolute paths in INFO logs.

These log lines now emit full file paths, which can leak user-identifying directory details (e.g., usernames/home paths). Prefer filename-only at INFO and keep full path for DEBUG when needed.

🔐 Safer logging patch
- log.info("Toggled uploaded flag to %s for %s", meta.uploaded, image_path)
+ log.info("Toggled uploaded flag to %s for %s", meta.uploaded, image_path.name)

- log.info("Toggled todo flag to %s for %s", meta.todo, image_path)
+ log.info("Toggled todo flag to %s for %s", meta.todo, image_path.name)

- log.info("Toggled edited flag to %s for %s", meta.edited, image_path)
+ log.info("Toggled edited flag to %s for %s", meta.edited, image_path.name)

- log.info("Toggled restacked flag to %s for %s", meta.restacked, image_path)
+ log.info("Toggled restacked flag to %s for %s", meta.restacked, image_path.name)

- log.info("Toggled favorite flag to %s for %s", meta.favorite, image_path)
+ log.info("Toggled favorite flag to %s for %s", meta.favorite, image_path.name)

- log.info("Toggled stacked flag to %s for %s", meta.stacked, image_path)
+ log.info("Toggled stacked flag to %s for %s", meta.stacked, image_path.name)

Also applies to: 2253-2253, 2276-2276, 2299-2299, 2317-2317, 2340-2340

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@faststack/app.py` at line 2230, The INFO logs currently emit full absolute
file paths (image_path), which can leak user directory info; change those
log.info calls to log the filename only by using os.path.basename(image_path)
(e.g., replace log.info("Toggled uploaded flag to %s for %s", meta.uploaded,
image_path) with log.info("Toggled uploaded flag to %s for %s", meta.uploaded,
os.path.basename(image_path))) and add a debug-level log that retains the full
path (e.g., log.debug("Full image path: %s", image_path)) so the detailed path
remains available for debugging; apply the same change to the other similar
occurrences that use image_path in INFO logs (the other log.info lines noted in
the comment).

Comment thread faststack/io/sidecar.py Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@faststack/imaging/jpeg.py`:
- Around line 13-18: The assignment from create_turbojpeg() to jpeg_decoder and
TURBO_AVAILABLE is being overwritten by the legacy initialization block that
sets JPEG_DECODER and TURBO_AVAILABLE later, making jpeg_decoder unused; update
the module to consistently use the discovery result by either (a) replacing the
legacy initialization block with a single assignment that sets uppercase
module-level names (set JPEG_DECODER = jpeg_decoder and TURBO_AVAILABLE =
TURBO_AVAILABLE returned by create_turbojpeg()), or (b) update all decode call
sites to reference the lowercase jpeg_decoder and a single TURBO_AVAILABLE
variable; ensure only one initialization path remains so create_turbojpeg()'s
detection controls availability on Windows and all decoder references
(JPEG_DECODER or jpeg_decoder) are consistently used.

In `@faststack/logging_setup.py`:
- Around line 45-47: get_app_data_dir() currently may return an unwritable path
which causes setup_logging() to attempt unguarded directory/file creation and
crash; modify get_app_data_dir() to verify writability (e.g., attempt to create
and remove a temp file or ensure os.access with write and try mkdir with
exist_ok) for the chosen candidate, and if none are writable fall back to a
known writable location such as a subdirectory under tempfile.gettempdir() (or
create a dedicated writable fallback), then return that verified path; also
update setup_logging() to handle exceptions from directory/file creation
gracefully (catch OSError when creating log dirs/files and fallback to stderr or
the writable temp fallback) so startup cannot fail on permission errors.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0adabc91-bc80-417e-851a-b47cd70f13bf

📥 Commits

Reviewing files that changed from the base of the PR and between 8b35bc8 and 6137a73.

📒 Files selected for processing (5)
  • README.md
  • faststack/imaging/jpeg.py
  • faststack/imaging/turbo.py
  • faststack/logging_setup.py
  • faststack/thumbnail_view/prefetcher.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • README.md

Comment thread faststack/imaging/jpeg.py Outdated
Comment thread faststack/logging_setup.py Outdated
@Dj-Shortcut
Copy link
Copy Markdown
Author

Addressed the merge conflict and pushed the updated branch. The remaining sidecar key/path work is being handled
separately in a follow-up PR, so PR #53 is now scoped back to the Windows turbojpeg DLL fallback changes.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
faststack/tests/test_turbo.py (1)

108-126: Consider saving and restoring original handlers for better test isolation.

The test correctly validates graceful degradation when file logging fails. However, the cleanup in finally clears all handlers rather than restoring the original state, which could affect subsequent tests if any had pre-existing handlers.

♻️ Optional: Save and restore handlers
     logging_setup.setup_logging(debug=True)
 
     root_logger = logging.getLogger()
+    original_handlers = root_logger.handlers.copy()
     try:
         assert any(
             isinstance(handler, logging.StreamHandler) for handler in root_logger.handlers
         )
     finally:
-        root_logger.handlers.clear()
+        root_logger.handlers[:] = original_handlers

Note: If the root logger starts empty in your test environment, the current approach is fine.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@faststack/tests/test_turbo.py` around lines 108 - 126, Update the
test_test_setup_logging_keeps_console_handler_when_file_logging_fails to save
the current root logger handlers before calling logging_setup.setup_logging and
restore them in the finally block instead of clearing all handlers; capture
root_logger.handlers (or a copy via list(root_logger.handlers)) prior to
setup_logging, run the assertions using logging_setup.setup_logging, and in
finally replace root_logger.handlers with the saved list so pre-existing
handlers are preserved for other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@faststack/tests/test_turbo.py`:
- Around line 108-126: Update the
test_test_setup_logging_keeps_console_handler_when_file_logging_fails to save
the current root logger handlers before calling logging_setup.setup_logging and
restore them in the finally block instead of clearing all handlers; capture
root_logger.handlers (or a copy via list(root_logger.handlers)) prior to
setup_logging, run the assertions using logging_setup.setup_logging, and in
finally replace root_logger.handlers with the saved list so pre-existing
handlers are preserved for other tests.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 213719a4-8d8a-45b8-9bb4-263d15a9f576

📥 Commits

Reviewing files that changed from the base of the PR and between 6137a73 and a38f055.

📒 Files selected for processing (4)
  • faststack/imaging/jpeg.py
  • faststack/imaging/turbo.py
  • faststack/logging_setup.py
  • faststack/tests/test_turbo.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • faststack/imaging/jpeg.py
  • faststack/logging_setup.py

@Dj-Shortcut
Copy link
Copy Markdown
Author

🧹 Nitpick comments (1)

faststack/tests/test_turbo.py (1)> 108-126: Consider saving and restoring original handlers for better test isolation.

The test correctly validates graceful degradation when file logging fails. However, the cleanup in finally clears all handlers rather than restoring the original state, which could affect subsequent tests if any had pre-existing handlers.

♻️ Optional: Save and restore handlers

     logging_setup.setup_logging(debug=True)
 
     root_logger = logging.getLogger()
+    original_handlers = root_logger.handlers.copy()
     try:
         assert any(
             isinstance(handler, logging.StreamHandler) for handler in root_logger.handlers
         )
     finally:
-        root_logger.handlers.clear()
+        root_logger.handlers[:] = original_handlers

Note: If the root logger starts empty in your test environment, the current approach is fine.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@faststack/tests/test_turbo.py` around lines 108 - 126, Update the
test_test_setup_logging_keeps_console_handler_when_file_logging_fails to save
the current root logger handlers before calling logging_setup.setup_logging and
restore them in the finally block instead of clearing all handlers; capture
root_logger.handlers (or a copy via list(root_logger.handlers)) prior to
setup_logging, run the assertions using logging_setup.setup_logging, and in
finally replace root_logger.handlers with the saved list so pre-existing
handlers are preserved for other tests.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@faststack/tests/test_turbo.py`:
- Around line 108-126: Update the
test_test_setup_logging_keeps_console_handler_when_file_logging_fails to save
the current root logger handlers before calling logging_setup.setup_logging and
restore them in the finally block instead of clearing all handlers; capture
root_logger.handlers (or a copy via list(root_logger.handlers)) prior to
setup_logging, run the assertions using logging_setup.setup_logging, and in
finally replace root_logger.handlers with the saved list so pre-existing
handlers are preserved for other tests.

ℹ️ Review info

Fixed in the current branch.

I verified the current code first: the test was clearing root_logger.handlers in finally, which could remove pre-
existing handlers and affect other tests. I updated it to save list(root_logger.handlers) before calling
setup_logging(), then restore that saved list in finally instead of clearing all handlers.

@Dj-Shortcut
Copy link
Copy Markdown
Author

Small note: this PR includes a tiny Windows-specific pytest test-infra hardening commit.

On Windows, pytest can fail with PermissionError (WinError 5) due to inaccessible
existing temp directories (e.g. var/pytest-temp, .pytest-local). This is unrelated
to the DLL/JPEG issue itself.

The change:

  • forces a clean basetemp (tmp/pytest-clean)
  • ignores legacy pytest temp/cache directories

No production code or test logic was modified.

If preferred, I can split this out into a separate PR, but I kept it here since it's
small and improves Windows test reliability for this branch.

@Dj-Shortcut
Copy link
Copy Markdown
Author

@CodeRabbit review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 20, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
faststack/imaging/turbo.py (1)

60-63: Filter non-existent PATH DLL candidates before enqueueing.

Every PATH directory currently becomes a TurboJPEG load attempt, which can create heavy exception/log noise on Windows fallback paths. Only add candidates when turbojpeg.dll actually exists.

♻️ Proposed change
-        for path_dir in os.getenv("PATH", "").split(os.pathsep):
-            if path_dir:
-                candidates.append(str(Path(path_dir) / "turbojpeg.dll"))
+        for path_dir in os.getenv("PATH", "").split(os.pathsep):
+            if not path_dir:
+                continue
+            dll_path = Path(path_dir) / "turbojpeg.dll"
+            if dll_path.is_file():
+                candidates.append(str(dll_path))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@faststack/imaging/turbo.py` around lines 60 - 63, The PATH scanning loop is
appending every PATH directory's turbojpeg.dll to the candidates list even if
the file doesn't exist, causing noisy load attempts; update the loop that builds
candidates (the for path_dir in os.getenv("PATH"... block) to check
Path(path_dir) / "turbojpeg.dll".exists() before calling candidates.append(...)
so only existing DLL paths are enqueued for loading.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@faststack/imaging/turbo.py`:
- Around line 60-63: The PATH scanning loop is appending every PATH directory's
turbojpeg.dll to the candidates list even if the file doesn't exist, causing
noisy load attempts; update the loop that builds candidates (the for path_dir in
os.getenv("PATH"... block) to check Path(path_dir) / "turbojpeg.dll".exists()
before calling candidates.append(...) so only existing DLL paths are enqueued
for loading.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2eb706e3-dfa0-4f32-a660-993ee6bd9001

📥 Commits

Reviewing files that changed from the base of the PR and between 6137a73 and 740397b.

📒 Files selected for processing (6)
  • .gitignore
  • faststack/imaging/jpeg.py
  • faststack/imaging/turbo.py
  • faststack/logging_setup.py
  • faststack/tests/test_turbo.py
  • pytest.ini
✅ Files skipped from review due to trivial changes (3)
  • .gitignore
  • pytest.ini
  • faststack/tests/test_turbo.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • faststack/logging_setup.py

@Dj-Shortcut Dj-Shortcut force-pushed the mijn-volgende-branch branch from 740397b to fc05bc4 Compare March 20, 2026 18:47
@AlanRockefeller
Copy link
Copy Markdown
Owner

Accepted changes from this PR (with modifications):

  1. Take the new imaging/turbo.py module — the centralized TurboJPEG discovery with Windows fallbacks is genuinely useful
  2. Update imaging/jpeg.py to use create_turbojpeg() from turbo.py, but keep existing comments intact
  3. Update thumbnail_view/prefetcher.py to use create_turbojpeg() from turbo.py
  4. Take the turbo.py tests from test_turbo.py (the create_turbojpeg tests, not the logging tests)
  5. Take the README TurboJPEG documentation

Reject:

  • logging_setup.py rewrite — current code is already better (fixed this up in the PR54 work)
  • Logging-related tests in test_turbo.py — test against a rewrite we're not taking
  • Gratuitous comment removal in jpeg.py — keep existing comments, they don't hurt anything and some are useful

The good parts of this PR will be merged with main in PR # 58

@Dj-Shortcut Dj-Shortcut deleted the mijn-volgende-branch branch March 25, 2026 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants