Add “Jump to Last Uploaded”, prioritize thumbnails, and improve shutdown safety#42
Add “Jump to Last Uploaded”, prioritize thumbnails, and improve shutdown safety#42AlanRockefeller merged 16 commits intomainfrom
Conversation
…en no linear-space edits are active (WB/exposure/highlights/shadows/clarity/texture/sharpness), and clamp export output to [0,1] on that path. Save performance: Avoid float_image.copy() during export when the edit set guarantees the pipeline won’t mutate the input buffer. Load performance: Apply EXIF orientation on the 8-bit Pillow path before float conversion (rotate uint8), and only rotate the float buffer on the 16-bit OpenCV path. Logging/robustness: Switch warnings/errors to lazy log formatting and improve load/save diagnostics.
WalkthroughThis PR delivers version 1.5.8 with significant enhancements: background deletion handling via typed deletion data structures and priority executors, favorites and flag-based filtering (uploaded, stacked, edited, restacked), batch auto-levels processing with UI progress tracking, improved cache thread-safety and path-based eviction, optimized fast-path image saving for level-only edits, daemon executor utilities for safe background threading, and extensive test coverage. Multiple debug/reproduction scripts are removed, and core modules like ImageEditor, ByteLRUCache, and ThumbnailModel are substantially refactored for robustness and performance. Changes
Sequence DiagramssequenceDiagram
participant User as User
participant UI as QML UI
participant UIState as UIState (Provider)
participant Controller as AppController
participant Editor as ImageEditor
participant Cache as Cache/<br/>Thumbnail
User->>UI: Click "Add to Batch" / "Run Batch Auto Levels"
UI->>UIState: batchAutoLevels()
UIState->>Controller: batch_auto_levels()
Controller->>Controller: Spawn deletion worker<br/>thread for batch
Controller->>UIState: Emit batchAutoLevelsProgress<br/>(current, total)
UIState->>UI: Update batchAutoLevelsCurrent
UI->>UI: Update progress bar
loop For each image in batch
Controller->>Editor: auto_levels(threshold_percent)
Editor->>Editor: Compute histogram,<br/>apply levels
Editor->>Cache: Invalidate affected<br/>thumbnail entries
Controller->>UIState: Emit progress(current++, total)
UIState->>UI: Progress signal
end
Controller->>UIState: Emit batchAutoLevelsFinished
UIState->>UI: Set batchAutoLevelsActive=false
UI->>UI: Close progress dialog,<br/>refresh thumbnails
sequenceDiagram
participant User as User
participant UI as QML UI
participant Controller as AppController
participant Worker as Delete Worker<br/>(Thread)
participant Cache as ByteLRUCache
participant Executor as Priority Executor
User->>UI: Delete image
UI->>Controller: delete_current_image()
Controller->>Controller: Optimistic: remove from<br/>image_files immediately
Controller->>Cache: evict_paths([image_path])
Controller->>UI: Emit gridRefresh,<br/>sync_ui_state
Controller->>Executor: submit(_delete_worker,<br/>job, priority=HIGH)
Executor->>Worker: Execute in background thread
Worker->>Worker: Move to recycle bin /<br/>handle failures
Worker->>Worker: Return result dict<br/>(status, successes,<br/>warnings, failures)
Worker-->>Executor: Future resolved
Controller->>Controller: _on_delete_finished(future)
alt Recycle failed
Controller->>Controller: _handle_delete_failures()<br/>prompt for permanent delete
Controller->>UI: Show permanent delete<br/>confirmation dialog
else Success / Partial
Controller->>Controller: Update undo history<br/>with completed job
end
UI->>UI: Reflect final state<br/>(restored or deleted)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/imaging/cache.py (1)
87-101:⚠️ Potential issue | 🟡 Minor
on_evictcallback may run under lock whenpopitemis triggered by__setitem__.When
super().__setitem__()(line 69) needs to make room, it callspopitem(). The callback runs after releasingpopitem'swith self._lockblock, but still inside__setitem__'s outer lock context (line 49). So the intent to run the callback outside the lock only holds for directpopitem()calls, not for eviction-triggered ones.This is unlikely to deadlock (RLock), but if
on_evictever does anything heavyweight or acquires other locks, it could cause contention. Worth a comment noting this subtlety.
🤖 Fix all issues with AI agents
In `@faststack/imaging/editor.py`:
- Around line 1895-1902: The method _ensure_float_image accesses and mutates
shared state (self.float_image and reads self.original_image) without holding
self._lock, causing races with the preview worker and callers like save_image;
fix this by acquiring self._lock around the check-and-assign sequence in
_ensure_float_image (i.e., wrap the early-return check, the original_image None
check/raise, and the conversion/assignment to self.float_image inside a critical
section using self._lock) so all reads/writes of these attributes are protected.
- Around line 1930-1939: The save_image method must acquire the instance lock
before reading or validating shared state: take self._lock (following the
pattern in get_preview_data_cached()) then call _ensure_float_image(), snapshot
self.current_edits and evaluate _edits_can_share_input(self.current_edits) while
holding the lock so the no-copy decision is race-free; set a local variable for
whether to copy and a local reference to self.float_image (or a copy if needed),
release the lock, and only then call _apply_edits(final_source,
for_export=True). Ensure you reference and protect current_edits and
float_image, and keep other methods like set_edit_param unchanged.
In `@faststack/reactive_test_output.txt`:
- Around line 1-15: The committed test output file
faststack/reactive_test_output.txt should be removed from the PR and excluded
from future commits; delete the file from the branch (or revert its addition)
and add an ignore rule such as *test_output.txt (or reactive_test_output.txt) to
.gitignore so similar snapshot/debug artifacts are not committed again, then run
git rm --cached if you need to stop tracking an already committed file before
committing the updated .gitignore.
In `@faststack/test_executors.py`:
- Around line 1-54: The file is a debug/reproduction script
(test_priority_executor) that misuses create_priority_executor (aliasing the
factory as PriorityExecutor), lacks pytest assertions, uses flaky sleep-based
synchronization, duplicates existing tests, and lives in the wrong directory;
fix by either removing this file entirely (and add **/test_executors.py to
.gitignore if it’s a local scratch file) OR convert it into a proper pytest test
under faststack/tests/ by calling the factory create_priority_executor(...)
correctly (don’t alias it as a class), replace print/if checks with assert
statements, replace time.sleep-based waits with synchronization on returned
Future objects (or use executor.shutdown(wait=True) and
future.result(timeout=...)) to deterministically wait for completion, and ensure
shutdown/cancel behavior is asserted via Future.cancelled(); reference
create_priority_executor, test_priority_executor, executor.submit, and
executor.shutdown when making changes.
In `@faststack/tests/debug_editor_error.py`:
- Around line 12-41: The test TestDebugError.test_debug_save_image is
ineffective: set up editor.original_image (e.g., assign a PIL Image or
MagicMock) so save_image proceeds past the early-return, patch Image.fromarray
at the lookup site faststack.imaging.editor.Image.fromarray (not
PIL.Image.fromarray), keep the create_backup_file patch and have the mocked
image's save raise PermissionError, remove all debug print statements, and
replace prints with a proper assertion using self.assertRaises(RuntimeError,
editor.save_image) (or with self.assertRaises in a context manager) to verify
save_image raises the expected RuntimeError.
In `@faststack/tests/repro_futures_cleanup.py`:
- Around line 18-20: The test setup erroneously assigns to the read-only
Path.suffix in the loop over image_files (created via
MagicMock(path=Path(...))), which will raise AttributeError; remove the for-loop
that does "for img in image_files: img.path.suffix = '.jpg'" (or replace it by
making img.path a MagicMock if you really need assignable attributes), leaving
image_files = [MagicMock(path=Path(f"test_{i}.jpg")) for i in range(10)] as the
correct setup for tests that expect ".jpg" suffix.
In `@faststack/tests/test_handle_failures_isolated.py`:
- Around line 25-85: The test copies production logic from
AppController._handle_delete_failures into
MockController._handle_delete_failures causing duplication and potential drift;
instead bind the real method onto the mock like test_handle_failures.py does
using AppController._handle_delete_failures.__get__(mock, MockController) so the
test uses the production implementation, and patch any external UI prompts
(confirm_permanent_delete or confirm_batch_permanent_delete) via a stub/mock so
the method can run in tests without user interaction.
In `@faststack/tests/test_refresh_crash.py`:
- Around line 6-19: The patches in the model fixture currently exit before tests
run because the with-block ends prior to returning the fixture; change the
fixture so the patch context stays active for the test run by yielding the model
from inside the patch context (or by creating patchers with patch(...) and
starting them before constructing the ThumbnailModel and stopping them after
yielding). Specifically update the fixture named model so the patch context that
wraps patch('faststack.thumbnail_view.model.find_images'),
patch('faststack.thumbnail_view.model.count_images_in_folder'), and
patch('faststack.thumbnail_view.model.read_folder_stats') remains active across
the yield, ensuring calls from ThumbnailModel.refresh(),
refresh_from_controller(), and _add_folders_to_entries() use the mocks during
test execution.
In `@faststack/tests/test_thumbnail_ready_emits_datachanged.py`:
- Around line 12-35: The test creates a ThumbnailModel (which subclasses
QAbstractListModel) before a Qt application exists, so add a session-scoped qapp
fixture that ensures a QCoreApplication instance is available (create one if
QCoreApplication.instance() is None) and then accept qapp as a parameter to the
existing thumbnail_model fixture (change def thumbnail_model(): to def
thumbnail_model(qapp):) so ThumbnailModel initialization and its signal/slot
connections occur with a valid QCoreApplication; reference ThumbnailModel,
thumbnail_model fixture, and QCoreApplication/qapp when making the edits.
In `@faststack/thumbnail_view/model.py`:
- Around line 378-388: The filtering currently assumes the metadata returned by
_get_metadata is a dict and uses meta.get(...), which causes AttributeError and
silently drops images; update the filter logic in the block that uses
self._active_filter_flags and self._get_metadata to first normalize the metadata
(reuse the existing _normalize_meta_flags helper) or coerce it to a mapping-like
dict before calling .get, and replace the bare except: pass with a warning log
(don’t swallow errors) so images aren’t silently excluded; apply the same fix
patterns to the similar code paths in refresh_from_controller and
_add_images_to_entries.
🟡 Minor comments (19)
faststack/repro_imports.py-1-13 (1)
1-13:⚠️ Potential issue | 🟡 MinorDebug script will ship in the package distribution.
This file sits inside
faststack/and thesetuptools.packages.findconfig (include = ["faststack*"]) will include it in the built package. If this is a throwaway repro script, move it outside the package (e.g., to a top-levelscripts/ortools/directory) or remove it before release.faststack/ui/keystrokes.py-41-41 (1)
41-41:⚠️ Potential issue | 🟡 Minor
Fshortcut not documented in README keyboard shortcuts.The
F→toggle_favoritebinding is added here but the README's keyboard shortcuts section (provided in context) doesn't list it. Consider adding it for discoverability.ChangeLog.md-28-28 (1)
28-28:⚠️ Potential issue | 🟡 MinorStray trailing backtick in changelog entry.
Line 28 ends with an unmatched backtick after "mistakes." which breaks the Markdown formatting.
Proposed fix
-- Refactor `ThumbnailModel` filtering into `set_filter()` with an active filter state; assert refresh runs on the GUI thread to catch threading mistakes.` +- Refactor `ThumbnailModel` filtering into `set_filter()` with an active filter state; assert refresh runs on the GUI thread to catch threading mistakes.faststack/tests/test_loupe_delete.py-65-87 (1)
65-87:⚠️ Potential issue | 🟡 MinorUseless ternary on line 84 — both branches are identical.
Both the
ifandelsebranches producelist(args[0]), making the condition a no-op.Proposed fix
- arg0 = list(args[0]) if not isinstance(args[0], (list, tuple, set)) else list(args[0]) + arg0 = list(args[0]).gitignore-88-89 (1)
88-89:⚠️ Potential issue | 🟡 MinorOverly broad glob patterns risk ignoring legitimate files.
**/*fail*.txtmatches any.txtfile containing "fail" anywhere in its name (e.g.,waterfall_data.txt), and**/*final*.txtmatches any containing "final" (e.g.,final_report.txt). Consider more specific patterns like**/*test_fail*.txtand**/*_final_*.txt, or scope them to specific directories.faststack/deletion_types.py-113-114 (1)
113-114:⚠️ Potential issue | 🟡 Minor
_to_pathwill createPath("")for empty-string values.If a worker dict contains
""(rather thanNone) for a missing path,_to_pathwill producePath("")instead ofNone. Depending on how callers check the result (e.g.,if record.jpg),Path("")is truthy and could cause downstream issues such asFileNotFoundErroror incorrect path operations.Proposed defensive fix
def _to_path(v): - return Path(v) if v is not None else None + return Path(v) if v else Nonefaststack/tests/thumbnail_view/test_prefetcher_priority.py-33-56 (1)
33-56:⚠️ Potential issue | 🟡 MinorTiming-dependent assertions may be flaky in CI.
The 20ms sleep (line 36) assumes the worker thread has started and pulled
med_0from the queue. On a loaded CI runner, thread scheduling latency could exceed this, causinghigh_0to be pulled beforemed_0or interleaved differently. Consider increasing the sleep or adding a synchronization primitive (e.g., anEventthat the mock sets when it starts processing) to make the test deterministic.Example: use an Event to confirm med_0 is being processed
import threading started = threading.Event() def mock_decode(path, path_hash, mtime_ns, size): if path.name == "med_0.jpg": started.set() time.sleep(0.1) finished_jobs.append(path.name) return b"fake_data" # ... after submitting med_0: started.wait(timeout=1.0) # Now safe to submit remaining jobsfaststack/qml/BatchProgressDialog.qml-41-46 (1)
41-46:⚠️ Potential issue | 🟡 MinorProgressBar
to: 0whenbatchAutoLevelsTotalis 0 may cause visual issues.If the batch starts before
batchAutoLevelsTotalis set (or is briefly 0),from: 0andto: 0can produce indeterminate behavior in the progress indicator. Consider guarding withMath.max(uiState.batchAutoLevelsTotal, 1).- to: uiState ? uiState.batchAutoLevelsTotal : 1 + to: uiState ? Math.max(uiState.batchAutoLevelsTotal, 1) : 1faststack/qml/BatchProgressDialog.qml-83-92 (1)
83-92:⚠️ Potential issue | 🟡 MinorConnections target may be null at initialization.
If
uiStateis not yet available when the dialog component is created,target: uiStatebeingnullwill generate QML warnings. Consider adding a guard:Connections { - target: uiState + target: uiState || null + enabled: uiState !== null && uiState !== undefinedfaststack/tests/test_exif_orientation.py-167-167 (1)
167-167:⚠️ Potential issue | 🟡 MinorUnused variable
res.The return value of
save_image()is assigned but never used. Either use it for validation or drop the assignment.Proposed fix
- res = self.editor.save_image(write_developed_jpg=True) + self.editor.save_image(write_developed_jpg=True)faststack/tests/test_reactive_delete.py-101-103 (1)
101-103:⚠️ Potential issue | 🟡 MinorUnused variable
future.Line 103 assigns
futurebut it's never referenced again. Ruff F841 correctly flags this.Proposed fix
- future = app_controller._delete_executor.submit.return_value + _ = app_controller._delete_executor.submit.return_value # noqa: F841Or simply remove the line if not needed for readability.
faststack/tests/test_reactive_delete.py-259-259 (1)
259-259:⚠️ Potential issue | 🟡 MinorUnused fixture argument
tmp_path.
test_undo_midflight_auto_restoresdeclarestmp_pathin its signature but usesapp_controller.image_dirinstead.Proposed fix
-def test_undo_midflight_auto_restores(app_controller, tmp_path): +def test_undo_midflight_auto_restores(app_controller):faststack/tests/test_deletion_unification.py-258-258 (1)
258-258:⚠️ Potential issue | 🟡 MinorTypo in test function name: "midlight" → "midflight".
test_cancel_midlight_restores_unprocessedshould betest_cancel_midflight_restores_unprocessed. The same typo also appears intest_reactive_delete.py(test_cancel_midlight_with_real_files).Proposed fix
-def test_cancel_midlight_restores_unprocessed(mock_controller): +def test_cancel_midflight_restores_unprocessed(mock_controller):faststack/tests/test_reactive_delete.py-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorUnused import:
time.
timeis imported but never used in this file.Proposed fix
-import timefaststack/thumbnail_view/model.py-469-524 (1)
469-524:⚠️ Potential issue | 🟡 MinorSame
_folder_counttiming issue inrefresh_from_controller.Line 519 also computes
_folder_countafterendResetModel(). Apply the same fix as inrefresh().faststack/tests/thumbnail_view/test_model.py-244-246 (1)
244-246:⚠️ Potential issue | 🟡 MinorUnused variable
base— dead code.
baseis assigned on Line 245 but never referenced inrefresh(). The confinement check is handled bynavigate_to(), so this leftover assignment should be removed.Proposed fix
def refresh(self) -> None: """ Rebuild the entries list based on filesystem + filters. """ cur = self.current_directory.resolve() - base = self.base_directory.resolve() folders: list[ThumbnailEntry] = []faststack/thumbnail_view/model.py-476-476 (1)
476-476:⚠️ Potential issue | 🟡 Minorf-string without placeholders.
Ruff F541: This f-string has no interpolation expressions. Use a plain string or include the thread details.
Proposed fix
- assert cur == own, f"ThumbnailModel refresh thread mismatch" + assert cur == own, ( + f"ThumbnailModel refresh thread mismatch: current={cur}, owner={own}" + )faststack/tests/thumbnail_view/test_model.py-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorMisleading file header comment.
Line 1 says
# faststack/thumbnail_view/model.pybut this file isfaststack/tests/thumbnail_view/test_model.py. This could confuse maintainers about which file they're editing.-# faststack/thumbnail_view/model.py +# faststack/tests/thumbnail_view/test_model.pyfaststack/thumbnail_view/model.py-352-409 (1)
352-409:⚠️ Potential issue | 🟡 Minor
_folder_countupdated afterendResetModel()— potential stale reads.Line 398 updates
self._folder_countafterendResetModel()(Line 396). Any slot connected tomodelResetthat readsfolder_countwill observe the old value. Consider moving the computation inside thetryblock beforeendResetModel().Proposed fix
+ self._folder_count = sum(1 for e in self._entries if e.is_folder) + finally: self.endResetModel() - self._folder_count = sum(1 for e in self._entries if e.is_folder) self.selectionChanged.emit()
🧹 Nitpick comments (44)
faststack/tests/debug_app_init.py (2)
1-6: Unused import:pytestis imported but never used.Line 1 imports
pytestwhich is not referenced anywhere in the file.Proposed fix
-import pytest -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, patch
12-37: Redundant QApplication block and unnecessary try/except/print pattern.
- Lines 23-29 duplicate the module-level QApplication guard (lines 8-10) and introduce an unused
qappvariable (Ruff F841).- The
try/except/raisepattern on lines 32-37 adds no value — if instantiation fails, the test fails with the exception traceback already. Theappvariable is also unused (Ruff F841).Proposed simplification
def test_app_init_only(): """Verify AppController can be instantiated with mocks.""" with patch("faststack.app.ByteLRUCache"), \ patch("faststack.app.ThumbnailModel"), \ patch("faststack.app.Prefetcher"), \ patch("faststack.app.PathResolver"), \ patch("faststack.app.Watcher"), \ patch("faststack.app.uuid"), \ patch("faststack.app.QTimer"), \ patch("faststack.app.concurrent.futures.ThreadPoolExecutor"): - - # Create QApplication instance - from PySide6.QtWidgets import QApplication - import sys - if not QApplication.instance(): - qapp = QApplication(sys.argv) - else: - qapp = QApplication.instance() - mock_engine = MagicMock() - try: - app = AppController(Path("."), mock_engine) - print("AppController instantiated successfully") - except Exception as e: - print(f"AppController instantiation failed: {e}") - raise + AppController(Path("."), mock_engine) # should not raisefaststack/tests/debug_exif.py (2)
1-6: Unused imports:sysandPath.Neither
sys(line 3) norPath(line 4) is used in this file.Proposed fix
import unittest -import sys -from pathlib import Path from PIL import Image, ExifTags from faststack.imaging.editor import ImageEditor, sanitize_exif_orientation
8-31: Debug test has no assertions and contains incomplete placeholder code.This test relies entirely on
assertorself.assert*calls), so it will always pass regardless of output. Lines 28-30 are explicitly marked as incomplete (# ... actually need real flow).Consider either:
- Adding actual assertions (e.g.,
self.assertIsNotNone(sanitized),self.assertEqual(chk.get(ExifTags.Base.Orientation), 1)) to make this a real test, or- Moving this out of the test suite if it's only meant for manual debugging.
Also, line 22 uses
if sanitized:which is falsy for emptybytes(b"")— useif sanitized is not None:if that distinction matters.faststack/test_prespawn_strategy.py (1)
1-34: Debug script ships in package, has no assertions, and leaks the executor.Three concerns:
Ships in distribution: Like
repro_imports.py, this sits insidefaststack/and will be packaged. Move to ascripts/directory orfaststack/tests/if it should be a real test.No assertions:
check_daemon()only prints — the test always passes. If the goal is to verify workers are daemon threads, assert it:def check_daemon(): - print(f"Thread {threading.current_thread().name} daemon: {threading.current_thread().daemon}") + return threading.current_thread().daemon- executor.submit(check_daemon).result() + assert executor.submit(check_daemon).result() is True, "Worker should be a daemon thread"
- Executor never shut down: Call
executor.shutdown(wait=False)at the end to avoid resource leaks.faststack/tests/test_loupe_delete.py (1)
140-145: Prefernext(iter(...))over single-element slice for dict keys.Line 142 creates an intermediate list just to get the first key.
Proposed fix
- job_id = list(mock_controller._pending_delete_jobs.keys())[0] + job_id = next(iter(mock_controller._pending_delete_jobs))faststack/repro_daemon_bug.py (1)
1-26: Debug/reproduction script — consider removing or moving out of the package.This script duplicates logic already covered by proper tests in
test_executor_shutdown.pyandtest_prespawn_strategy.py. Shipping it in thefaststack/package means it gets installed with the application. If it's still needed for reference, consider moving it to ascripts/ordebug/directory outside the package, or converting it to a proper pytest test.Also, the
timeimport (line 3) is unused.faststack/tests/test_config_setters.py (1)
94-118: Clean up leftover developer reasoning comments.Lines 98–105 contain stream-of-consciousness notes about mock behavior. These read like debugging notes rather than test documentation. Consider replacing with a concise one-liner explaining the expected default.
Proposed cleanup
def test_set_auto_level_strength(self): self.mock_config.set.reset_mock() self.mock_config.save.reset_mock() - # Default was 1.0 in code, but our mock config.getfloat returns 0.1 (as per setUp) - # Wait, if config.getfloat returned 0.1 for threshold, did it return 0.1 for strength too? - # Yes, line 62 in original: mock_config_obj.getfloat.return_value = 0.1 - # In setUp I set it to 0.1. - - # But wait, config.getfloat is called with default 1.0 for strength in app.py: - # self.auto_level_strength = config.getfloat("core", "auto_level_strength", 1.0) - # If I mock getfloat to always return 0.1, then it's 0.1. - + # mock_config.getfloat always returns 0.1 (setUp), so initial strength is 0.1 + new_val = 0.8faststack/tests/test_jump_to_last_uploaded.py (1)
7-14: Duplicatedqappfixture — consider centralizing inconftest.py.This session-scoped
qappfixture is defined identically intest_loupe_delete.pyand here. Moving it to a sharedconftest.pywould avoid duplication and ensure consistent QApplication lifecycle across all tests.faststack/io/utils.py (1)
8-21:normalize_path_keycorrectly handles absolute paths; document that it expects absolute inputs.
os.path.abspathresolves relative paths against the current working directory. However, all callers in the codebase pass absolute paths:os.scandir()returns absolute paths, andimage_diris always validated and absolute. No practical risk exists, but the function would be more defensive if it either documented the absolute-path precondition in its docstring or converted unexpected relative paths usingPath().resolve()instead ofos.path.abspath()..gitignore (1)
11-12: Ignoring the entiredocs/directory is risky long-term.The comment says "We don't have any good docs yet," but this will silently swallow any future documentation contributions. Consider tracking a TODO or using a more targeted pattern so this doesn't become a forgotten footgun.
faststack/imaging/cache.py (1)
194-211: Clean up leftover thinking/deliberation comments.Lines 196–203 contain stream-of-consciousness developer notes (
"We need size before removal…","We want to log *how much*…", etc.). These should be removed or condensed to a single concise comment.♻️ Suggested cleanup
# 4. Remove keys removed_bytes = 0 for k in keys_to_remove: - # We need size before removal to log correctly? - # LRUCache.pop returns value. We can ask getsizeof(value) but pop removes it anyway. - # ByteLRUCache tracks currsize. We can diff currsize. - # But simpler: just trust currsize updates. - # We want to log *how much* we removed. - # Accessing self.getsizeof(val) needs val. - # val = self.pop(k) would work. - # We want to log *how much* we removed. if k in self: - # Pop first to avoid updating LRU order with self[k] val = self.pop(k) try: size = get_decoded_image_size(val)faststack/util/executors.py (2)
124-143: Uselog.exceptioninstead oflog.errorfor traceback visibility.Line 137 uses
log.error(...)which won't include the traceback. Since this catches aBaseExceptionin a worker loop, the traceback is valuable for debugging.♻️ Proposed fix
- log.error("Error in PriorityExecutor worker: %s", e) + log.exception("Error in PriorityExecutor worker: %s", e)
194-199: 1-second join timeout may silently leave workers running.If a worker is stuck on a long task,
t.join(timeout=1.0)returns without the thread being joined, and there's no log indicating the thread is still alive. Since workers are daemon threads they'll die with the process, but logging a warning would help debugging slow shutdowns.♻️ Proposed enhancement
if wait: for t in self._workers: try: t.join(timeout=1.0) + if t.is_alive(): + log.warning("Worker %s did not terminate within timeout", t.name) except Exception: passfaststack/imaging/prefetch.py (3)
122-122: Defined_EXIF_ORIENTATION_TAGconstant is unused; literal274is used instead.Line 122 defines
_EXIF_ORIENTATION_TAG = 274, but line 530 uses the literal274. Use the constant for consistency and self-documentation.♻️ Proposed fix at line 530
- orientation = exif_obj.get(274, 1) if exif_obj else 1 + orientation = exif_obj.get(_EXIF_ORIENTATION_TAG, 1) if exif_obj else 1Also applies to: 526-530
425-432: Silentexcept Exception: passswallows EXIF metadata errors without any logging.Lines 431–432 and 500–501 catch all exceptions and silently pass when extracting EXIF data from mmap'd images. While this is defensive, it makes debugging metadata-related issues very difficult. A
log.debugwould preserve the resilience while aiding troubleshooting.♻️ Proposed fix (example for lines 431-432; apply similar at 500-501)
- except Exception: - pass + except Exception as e: + log.debug("Failed to extract metadata from mmap: %s", e)Also applies to: 495-501
375-564:_decode_and_cacheis ~190 lines with deeply nested try/except — consider extracting helpers.The ICC path, non-ICC JPEG path, Pillow fallback, EXIF orientation, and saturation compensation could each be small helper methods. This would improve readability and make individual code paths easier to test. Not urgent, but would pay off as this code evolves.
faststack/tests/repro_futures_cleanup.py (2)
54-59:simulate_cleanupduplicates_cleanup_futurelogic — call the real method instead.If
_cleanup_future's implementation changes, this test won't detect regressions. Callprefetcher._cleanup_future(index, future_a)directly.♻️ Proposed fix
- def simulate_cleanup(prefetcher, idx, fut): - with prefetcher._futures_lock: - if prefetcher.futures.get(idx) is fut: - del prefetcher.futures[idx] - - simulate_cleanup(prefetcher, index, future_a) + prefetcher._cleanup_future(index, future_a)And similarly for line 65:
- simulate_cleanup(prefetcher, index, future_b) + prefetcher._cleanup_future(index, future_b)
9-11:sys.modulesinjection is fragile — preferunittest.mock.patchor pytestmonkeypatch.Directly manipulating
sys.modulesbefore import is brittle and can leak across tests if module state isn't restored. Using@patch.dict(sys.modules, ...)or pytest'smonkeypatchis safer and self-cleaning.faststack/tests/test_executor_shutdown.py (1)
11-48: Drain test asserts presence but not LIFO order — comments are misleading.Lines 40-44 discuss LIFO ordering but the assertions only verify all three values are present. If the intent is to verify LIFO ordering within the same priority, consider asserting the order of
results. Otherwise, remove the LIFO discussion comments to avoid confusion.faststack/deletion_types.py (1)
95-96: Barelisttype annotations are inconsistent with the rest of the file.
perm_successandperm_failuse unparameterizedlistwhile every other collection field usesList[...]with explicit element types. Similarly,saved_batchesonDeleteJob(line 46) usesOptional[list]. Consider usingList[Tuple[int, Any]](per the comments) for consistency and self-documentation.Proposed fix
- perm_success: list = field(default_factory=list) # List[(idx, ImageFile)] - perm_fail: list = field(default_factory=list) # List[(idx, ImageFile)] + perm_success: List[Tuple[int, Any]] = field(default_factory=list) + perm_fail: List[Tuple[int, Any]] = field(default_factory=list)faststack/tests/test_exif_compat.py (1)
16-20: Remove commented-out code and verbose rationale comments.Lines 16-18 read like internal instructions ("The instruction implies…") rather than code comments — they'll confuse future readers. Lines 19-20 are dead code. Since the intent is to stop mocking
faststack.models, clean this up by removing all five lines entirely (or at most leave a one-line note explaining why the mock was removed).Proposed cleanup
# Pre-mock modules that might cause issues or aren't needed for this test sys.modules["cv2"] = MagicMock() -# Mock faststack.models since it's used by editor.py -# The instruction implies removing 'faststack.models' from a patch.dict, -# but it's currently a direct assignment. -# Assuming the intent is to remove the mocking of faststack.models entirely. -# mock_models = MagicMock() -# sys.modules["faststack.models"] = mock_models +# faststack.models is no longer mocked; tests use real model definitions.faststack/models.py (1)
72-72: Consider whether afavorite_datefield is needed for consistency.Every other boolean flag in
EntryMetadata(stacked,uploaded,edited,restacked) has a companion*_date: Optional[str]field. If there's no need to track when an item was favorited (e.g., for sorting or display), this omission is fine — just worth a conscious note.faststack/tests/test_editor_no_copy.py (1)
110-114:*argsafter keyword argumentfor_exportis dead code (B026).In
spy_apply(arr, for_export=False, *args, **kwargs),*argscan never capture any values becausefor_exportalready consumes the second positional argument. The real_apply_editssignature takes(self, source_arr, for_export=True), so justarrandfor_exportsuffice.Proposed fix
- def spy_apply(arr, for_export=False, *args, **kwargs): - # We only care about the call for this specific test instance - if for_export and arr is ed.float_image: - seen["same_obj"] = True - return real_apply(arr, for_export=for_export, *args, **kwargs) + def spy_apply(arr, for_export=False, **kwargs): + if for_export and arr is ed.float_image: + seen["same_obj"] = True + return real_apply(arr, for_export=for_export, **kwargs)faststack/thumbnail_view/provider.py (1)
225-226: Use%-style formatting inlog.debugfor lazy evaluation.f-strings are always evaluated, even when the DEBUG level is disabled, which adds overhead in a method iterating over all model entries.
- log.debug(f"PathResolver update took {dt*1000:.2f}ms for {model.rowCount()} items") + log.debug("PathResolver update took %.2fms for %d items", dt * 1000, model.rowCount())faststack/tests/thumbnail_view/test_prefetcher_priority.py (1)
61-100: LIFO test has similar timing fragility.The 10ms sleeps between job submissions (lines 81, 85, 87) serve as sequencing hints but the LIFO ordering is actually determined by the
-seqcounter inPriorityExecutor, not by submission timing. The sleeps can likely be removed without affecting correctness, which would also make the test faster.faststack/tests/test_new_features.py (1)
22-28: Heuristicmaxv <= 1.5for detecting normalized floats can misclassify very dark uint8-scale images.If an image in the [0, 255] range has all pixel values ≤ 1.5 (e.g., a nearly-black image), this would incorrectly scale by 255×. For the current synthetic test images this is fine, but consider checking
arr.max() <= 1.0or passing an explicit flag if this utility is reused elsewhere.faststack/tests/test_refresh_optimization.py (1)
7-13: Session-scopedqappfixture may conflict with other test modules.If other test files also create a
QApplication, the session scope means only one instance exists. This is correct behavior (Qt allows only one QApplication), but consider extracting this fixture to a sharedconftest.pyto avoid duplication across test files.faststack/tests/test_raw_pipeline.py (1)
31-39:DummyImageFilebehavior diverges from realImageFile.
has_rawchecks.exists()(real model only checksis not None), andraw_pathraisesAttributeError(real model returnsNone). This is fine for the tests in this file, but be aware that code exercised through these tests may behave differently in production if it relies onraw_pathreturningNoneorhas_rawnot touching the filesystem.faststack/tests/test_handle_failures_isolated.py (1)
2-2: Unused importsys.
sysis imported but never used in this file.faststack/tests/test_deletion_perf_structure.py (2)
16-42: Unusedmock_appfixture.This fixture is defined but no test function uses it — each test sets up its own patching inline. Either remove it or refactor the tests to use it.
78-81: Remove debug print statements.Lines 78 and 81 contain
print("DEBUG: ...")statements that should be removed or converted to logging before merge.Proposed fix
- print("DEBUG: Calling _delete_indices") # Act summary = app._delete_indices([0], "test") - print("DEBUG: Returned from _delete_indices")faststack/qml/FilterDialog.qml (1)
143-145: EmptyonAcceptedhandler can be removed.Since flags and filter string are both collected live, this empty handler serves no purpose.
Proposed fix
- onAccepted: { - // Flags are now collected live via onCheckedChanged - } -faststack/tests/test_deletion_unification.py (2)
376-389: Inconsistent indentation insidewithblock.Lines 378–389 are indented with 12 spaces (3 levels) inside the
withblock, but thewithstatement at line 438 in a similar pattern uses 8 spaces. The extra indentation here doesn't break anything but is inconsistent with the rest of the file.
421-423: Usenext(iter(...))instead oflist(...)[0]per Ruff RUF015.
list(mock_controller._pending_delete_jobs.keys())[0]materializes the entire dict view into a list just to get the first key.Proposed fix
- job_id = list(mock_controller._pending_delete_jobs.keys())[0] + job_id = next(iter(mock_controller._pending_delete_jobs))faststack/tests/test_skip_linear.py (1)
16-20: Consider usingnp.random.default_rng(42)instead ofnp.random.seed(42).
np.random.seed/np.random.randuse the legacy global RNG. With numpy 2.0+,np.random.default_rng(42)is preferred. Not urgent since this is test-only code and the legacy API still works.Proposed fix
def setUp(self): - np.random.seed(42) self.editor = ImageEditor() + rng = np.random.default_rng(42) # Deterministic float32 image in [0.1, 0.9] — avoids clip boundaries - self.arr = (np.random.rand(100, 100, 3).astype(np.float32) * 0.8 + 0.1) + self.arr = (rng.random((100, 100, 3), dtype=np.float32) * 0.8 + 0.1)faststack/thumbnail_view/prefetcher.py (3)
118-120:size: int = Noneshould use explicitOptionalor union type.PEP 484 prohibits implicit
Optional. SinceNoneis a valid default, the type hint should reflect this.Proposed fix
def submit( - self, path: Path, mtime_ns: int, size: int = None, priority: int = PRIO_MED + self, path: Path, mtime_ns: int, size: Optional[int] = None, priority: int = PRIO_MED ) -> bool:
71-71: Same implicitOptionalissue in__init__.
max_workers: int = Nonehas the same PEP 484 issue.Proposed fix
- max_workers: int = None, + max_workers: Optional[int] = None,
370-378:_order.remove(key)inget()is O(n) — considercollections.OrderedDictfor the LRU cache.
ThumbnailCache.get()callsself._order.remove(key)which scans the list linearly. Withmax_items=5000, this could become a hot-path bottleneck under heavy thumbnail scrolling. AnOrderedDict(ormove_to_end()) would give O(1) LRU promotion. This is existing code, so not blocking, but worth considering as a follow-up.faststack/thumbnail_view/model.py (3)
239-246: Tight coupling:_get_loupe_index_for_entryreaches into parent's private_path_to_index.This couples the model to the AppController's internal structure. If the parent changes its attribute name or type, this silently returns
Nonefor all entries, breakingIsInBatchRoleandIsCurrentRole. Consider injecting a callback (likeget_batch_indices_callback) instead of relying onhasattr.
749-754:_compute_path_hashis a trivial wrapper — consider removing.This method only delegates to
compute_path_hash(path). All other call sites in this file already usecompute_path_hashdirectly (Lines 212, 276, 590). This wrapper adds no value and could be removed to reduce surface area.
526-575: Duplicated metadata extraction logic across_add_images_to_entriesandrefresh.Lines 538-561 repeat the same metadata key extraction (
stacked,uploaded,edited,restacked,favorite) as the filtering block inrefresh(Lines 378-388). Extracting a shared helper (like the test model's_normalize_meta_flags) would reduce duplication and ensure consistency.faststack/imaging/editor.py (2)
927-934: Skip-linear export path:_skip_linearis only active forfor_export=True.This ensures preview rendering always goes through the full linear pipeline (needed for UI highlight analysis), while export skips it when safe. The
log.debugat Line 934 is useful for debugging but runs unconditionally during export — consider guarding it behind_debugfor consistency with other timing logs.
2060-2062:log.exceptionalready includes the traceback — passingeis redundant.Ruff TRY401:
log.exception()automatically appends the exception info from the current handler. Theein the format string is superfluous.Proposed fix
- log.exception("Failed to save %s: %s", self.current_filepath, e) + log.exception("Failed to save %s", self.current_filepath)
| def _ensure_float_image(self) -> None: | ||
| """Ensure self.float_image exists. Needed when load_image(preview_only=True).""" | ||
| if self.float_image is not None: | ||
| return | ||
| if self.original_image is None: | ||
| raise RuntimeError("No image loaded") | ||
| rgb = self.original_image.convert("RGB") | ||
| self.float_image = np.array(rgb).astype(np.float32) / 255.0 |
There was a problem hiding this comment.
_ensure_float_image is not protected by self._lock.
This method reads and writes self.float_image and reads self.original_image without acquiring self._lock. Since save_image (which calls this) can potentially race with the preview worker that reads float_image under the lock, consider wrapping the check-and-assign in the lock:
Proposed fix
def _ensure_float_image(self) -> None:
"""Ensure self.float_image exists. Needed when load_image(preview_only=True)."""
- if self.float_image is not None:
- return
- if self.original_image is None:
- raise RuntimeError("No image loaded")
- rgb = self.original_image.convert("RGB")
- self.float_image = np.array(rgb).astype(np.float32) / 255.0
+ with self._lock:
+ if self.float_image is not None:
+ return
+ if self.original_image is None:
+ raise RuntimeError("No image loaded")
+ rgb = self.original_image.convert("RGB")
+ self.float_image = np.array(rgb).astype(np.float32) / 255.0🧰 Tools
🪛 Ruff (0.15.0)
[warning] 1900-1900: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@faststack/imaging/editor.py` around lines 1895 - 1902, The method
_ensure_float_image accesses and mutates shared state (self.float_image and
reads self.original_image) without holding self._lock, causing races with the
preview worker and callers like save_image; fix this by acquiring self._lock
around the check-and-assign sequence in _ensure_float_image (i.e., wrap the
early-return check, the original_image None check/raise, and the
conversion/assignment to self.float_image inside a critical section using
self._lock) so all reads/writes of these attributes are protected.
|
|
||
| # 1. Apply Edits to Full Resolution | ||
| # Skip the expensive .copy() when safe — see _edits_can_share_input(). | ||
| _safe_no_copy = self._edits_can_share_input(self.current_edits) | ||
| source_arr = self.float_image if _safe_no_copy else self.float_image.copy() | ||
| if _safe_no_copy: | ||
| log.debug("save_image: skipping float_image.copy() (safe no-copy path)") | ||
| final_float = self._apply_edits( | ||
| self.float_image.copy(), for_export=True | ||
| source_arr, for_export=True | ||
| ) # (H,W,3) float32 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all calls to save_image to understand usage patterns
rg -n "\.save_image\(" --type=py -B2 -A2Repository: AlanRockefeller/faststack
Length of output: 6331
🏁 Script executed:
#!/bin/bash
# Check the thread assertion in refresh() at lines 354-356
sed -n '350,360p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 652
🏁 Script executed:
#!/bin/bash
# Look at the get_preview_data_cached implementation around line 1514
sed -n '1510,1525p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 620
🏁 Script executed:
#!/bin/bash
# Check the _edits_can_share_input method to understand the safe no-copy logic
rg -n "_edits_can_share_input" --type=py -B2 -A10Repository: AlanRockefeller/faststack
Length of output: 14699
🏁 Script executed:
#!/bin/bash
# Check for any lock mechanisms or thread safety decorators in the class
rg -n "Lock\|lock\|thread\|@thread" --type=py faststack/imaging/editor.py | head -30Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check refresh() method for thread assertions
sed -n '340,370p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 1895
🏁 Script executed:
#!/bin/bash
# Get context around the background worker in app.py
sed -n '1190,1210p' faststack/app.pyRepository: AlanRockefeller/faststack
Length of output: 1031
🏁 Script executed:
#!/bin/bash
# Search for locks protecting image_editor in app.py
rg -n "Lock\|lock\|thread\|Thread\|Mutex" faststack/app.py | grep -i "image_editor\|edit" | head -20Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check if image_editor operations have locks/synchronization
rg -n "self\.image_editor" faststack/app.py | head -10Repository: AlanRockefeller/faststack
Length of output: 679
🏁 Script executed:
#!/bin/bash
# Look for any mutex or lock protecting float_image or edits
rg -n "_lock\|_mutex\|with.*lock\|threading" faststack/imaging/editor.py | head -20Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Find _lock definition in ImageEditor
rg -n "_lock" faststack/imaging/editor.py | head -20Repository: AlanRockefeller/faststack
Length of output: 766
🏁 Script executed:
#!/bin/bash
# Check the complete save_image method implementation
sed -n '1900,1950p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 2106
🏁 Script executed:
#!/bin/bash
# Look for __init__ to see where _lock is initialized
rg -n "def __init__" faststack/imaging/editor.py -A20 | head -40Repository: AlanRockefeller/faststack
Length of output: 967
🏁 Script executed:
#!/bin/bash
# Check if save_image has any lock usage
sed -n '1900,2000p' faststack/imaging/editor.py | grep -n "lock\|Lock\|with"Repository: AlanRockefeller/faststack
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Check the ENTIRE save_image method to see if it uses _lock
awk '/def save_image/,/^ def [a-z_]/ {print NR": "$0}' faststack/imaging/editor.py | head -150Repository: AlanRockefeller/faststack
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Get exact line count of save_image method
rg -n "def save_image" faststack/imaging/editor.py -A200 | tail -5Repository: AlanRockefeller/faststack
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Get the ENTIRE save_image method (lines 1904-2063)
sed -n '1904,2063p' faststack/imaging/editor.py | head -160Repository: AlanRockefeller/faststack
Length of output: 7078
🏁 Script executed:
#!/bin/bash
# Check if there's any synchronization around the save_image call in app.py
sed -n '1180,1220p' faststack/app.pyRepository: AlanRockefeller/faststack
Length of output: 1865
🏁 Script executed:
#!/bin/bash
# Check if get_preview_data_cached uses locks (as it's the competing reader)
sed -n '1500,1530p' faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 1244
🏁 Script executed:
#!/bin/bash
# Verify how other methods that access float_image/current_edits use locks
rg -n "def get_preview_data" faststack/imaging/editor.py -A15 | head -30Repository: AlanRockefeller/faststack
Length of output: 1333
🏁 Script executed:
#!/bin/bash
# Verify if _apply_edits actually mutates the input array on the no-copy safe path
sed -n '1300,1350p' faststack/imaging/editor.py | head -60Repository: AlanRockefeller/faststack
Length of output: 2117
🏁 Script executed:
#!/bin/bash
# Check if float_image is ever modified after initialization
rg -n "self\.float_image\s*=" faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 327
Add lock protection to save_image method.
save_image is called from both background threads (app.py:1198) and the main UI thread (app.py:5713, 5945, 6039, 6177) without acquiring _lock, while other methods like set_edit_param() (line 1566) and get_preview_data_cached() (line 1505) correctly protect access to self.current_edits and self.float_image with locks.
The no-copy path's safety check _edits_can_share_input(self.current_edits) at line 1933 reads edits without lock protection, creating a race condition where current_edits could change between the safety check and the call to _apply_edits. Additionally, _ensure_float_image() (called at line 1925) modifies self.float_image at line 1902 without lock protection.
Follow the pattern in get_preview_data_cached(): acquire _lock upfront, snapshot both current_edits and validate the no-copy safety check under the lock, then release the lock before the expensive _apply_edits call.
🤖 Prompt for AI Agents
In `@faststack/imaging/editor.py` around lines 1930 - 1939, The save_image method
must acquire the instance lock before reading or validating shared state: take
self._lock (following the pattern in get_preview_data_cached()) then call
_ensure_float_image(), snapshot self.current_edits and evaluate
_edits_can_share_input(self.current_edits) while holding the lock so the no-copy
decision is race-free; set a local variable for whether to copy and a local
reference to self.float_image (or a copy if needed), release the lock, and only
then call _apply_edits(final_source, for_export=True). Ensure you reference and
protect current_edits and float_image, and keep other methods like
set_edit_param unchanged.
| ============================= test session starts ============================= | ||
| platform win32 -- Python 3.12.10, pytest-9.0.2, pluggy-1.6.0 -- C:\code\faststack\faststack\verify_venv\Scripts\python.exe | ||
| rootdir: C:\code\faststack | ||
| configfile: pyproject.toml | ||
| collecting ... collected 7 items | ||
|
|
||
| tests\test_reactive_delete.py::test_optimistic_ui_removal PASSED [ 14%] | ||
| tests\test_reactive_delete.py::test_undo_pending_delete_no_disk_ops PASSED [ 28%] | ||
| tests\test_reactive_delete.py::test_async_delete_completion PASSED [ 42%] | ||
| tests\test_reactive_delete.py::test_delete_rollback_on_cancel PASSED [ 57%] | ||
| tests\test_reactive_delete.py::test_debounced_refresh PASSED [ 71%] | ||
| tests\test_reactive_delete.py::test_cancel_midlight_with_real_files PASSED [ 85%] | ||
| tests\test_reactive_delete.py::test_undo_then_completion_no_bookkeeping PASSED [100%] | ||
|
|
||
| ============================== 7 passed in 0.56s ============================== |
There was a problem hiding this comment.
Remove test output file from version control.
Test output snapshots should not be committed to the repository. This file:
- Contains environment-specific paths (
C:\code\faststack\faststack\verify_venv\Scripts\python.exe) - Becomes stale immediately after each test run
- Adds no lasting value to the codebase
- Appears to be a debugging artifact
Consider:
- Remove this file from the PR
- Add
*test_output.txtor similar patterns to.gitignore - For tracking test results, use CI/CD reporting, coverage badges, or test result artifacts in your CI pipeline instead
🤖 Prompt for AI Agents
In `@faststack/reactive_test_output.txt` around lines 1 - 15, The committed test
output file faststack/reactive_test_output.txt should be removed from the PR and
excluded from future commits; delete the file from the branch (or revert its
addition) and add an ignore rule such as *test_output.txt (or
reactive_test_output.txt) to .gitignore so similar snapshot/debug artifacts are
not committed again, then run git rm --cached if you need to stop tracking an
already committed file before committing the updated .gitignore.
| import time | ||
| import threading | ||
| from faststack.util.executors import create_priority_executor as PriorityExecutor | ||
|
|
||
| def test_priority_executor(): | ||
| print("Testing PriorityExecutor...") | ||
| executor = PriorityExecutor(max_workers=1, thread_name_prefix="Test") | ||
|
|
||
| results = [] | ||
| def task(name, delay=0.1): | ||
| time.sleep(delay) | ||
| results.append(name) | ||
| return name | ||
|
|
||
| # Fill the worker and wait a bit to ensure it STARTS | ||
| executor.submit(task, "initial", delay=0.2) | ||
| time.sleep(0.05) | ||
|
|
||
| # Submit tasks with different priorities and see execution order | ||
| # Lower number = higher priority | ||
| # within same priority, higher sequence = higher priority (LIFO) | ||
| executor.submit(task, "p2_a", priority=2) | ||
| executor.submit(task, "p2_b", priority=2) | ||
| executor.submit(task, "p1", priority=1) | ||
|
|
||
| print("Tasks submitted, waiting for completion...") | ||
| # Expected order: "initial" (already running), "p1", "p2_b", "p2_a" | ||
|
|
||
| time.sleep(1.0) | ||
| print("Execution order:", results) | ||
|
|
||
| expected = ["initial", "p1", "p2_b", "p2_a"] | ||
| if results == expected: | ||
| print("SUCCESS: Priority and LIFO ordering correct.") | ||
| else: | ||
| print(f"FAILURE: Expected {expected}, got {results}") | ||
|
|
||
| print("\nTesting shutdown and cancellation...") | ||
| executor = PriorityExecutor(max_workers=1, thread_name_prefix="TestShutdown") | ||
| executor.submit(task, "long", delay=0.5) | ||
| f1 = executor.submit(task, "queued1") | ||
| f2 = executor.submit(task, "queued2") | ||
|
|
||
| executor.shutdown(wait=True, cancel_futures=True) | ||
| print(f"f1 cancelled: {f1.cancelled()}") | ||
| print(f"f2 cancelled: {f2.cancelled()}") | ||
|
|
||
| if f1.cancelled() and f2.cancelled(): | ||
| print("SUCCESS: Futures cancelled on shutdown.") | ||
| else: | ||
| print("FAILURE: Futures not cancelled.") | ||
|
|
||
| if __name__ == "__main__": | ||
| test_priority_executor() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
This appears to be a debug/reproduction script, not a proper test.
Several issues:
- Misleading alias (line 3):
create_priority_executor as PriorityExecutor— this is a factory function, not the class itself. - No assertions: Uses
print/ifinstead ofassert, so pytest would mark this as passing even when results are wrong. - Sleep-based synchronization (line 29):
time.sleep(1.0)is inherently flaky. - Location: Lives in
faststack/rather thanfaststack/tests/. - Duplicate coverage:
faststack/tests/test_executor_shutdown.pyalready covers priority ordering and cancellation with proper pytest patterns.
Consider removing this file (and adding **/test_executors.py to .gitignore if it's a local scratch file) or converting it to a proper pytest test in faststack/tests/.
🤖 Prompt for AI Agents
In `@faststack/test_executors.py` around lines 1 - 54, The file is a
debug/reproduction script (test_priority_executor) that misuses
create_priority_executor (aliasing the factory as PriorityExecutor), lacks
pytest assertions, uses flaky sleep-based synchronization, duplicates existing
tests, and lives in the wrong directory; fix by either removing this file
entirely (and add **/test_executors.py to .gitignore if it’s a local scratch
file) OR convert it into a proper pytest test under faststack/tests/ by calling
the factory create_priority_executor(...) correctly (don’t alias it as a class),
replace print/if checks with assert statements, replace time.sleep-based waits
with synchronization on returned Future objects (or use
executor.shutdown(wait=True) and future.result(timeout=...)) to
deterministically wait for completion, and ensure shutdown/cancel behavior is
asserted via Future.cancelled(); reference create_priority_executor,
test_priority_executor, executor.submit, and executor.shutdown when making
changes.
| class TestDebugError(unittest.TestCase): | ||
| def test_debug_save_image(self): | ||
| editor = ImageEditor() | ||
| editor.float_image = np.zeros((10, 10, 3), dtype=np.float32) | ||
| editor.current_filepath = Path("fake_path.jpg") | ||
|
|
||
| # Patch create_backup_file to succeed | ||
| with patch("faststack.imaging.editor.create_backup_file", return_value=Path("backup.jpg")): | ||
| # Patch Image.fromarray to return a mock that fails to save | ||
| mock_img = MagicMock() | ||
| mock_img.save.side_effect = PermissionError("Mocked save error") | ||
|
|
||
| print(f"DEBUG: Real Image.fromarray before patch: {Image.fromarray}") | ||
|
|
||
| with patch("PIL.Image.fromarray", return_value=mock_img) as mock_fromarray: | ||
| print(f"DEBUG: Image.fromarray is patched: {Image.fromarray}") | ||
| print(f"DEBUG: mock_fromarray: {mock_fromarray}") | ||
|
|
||
| # Verify that calling Image.fromarray returns our mock | ||
| img = Image.fromarray(np.zeros((10,10,3), dtype=np.uint8)) | ||
| print(f"DEBUG: Returned img: {img}") | ||
| print(f"DEBUG: img.save side effect: {img.save.side_effect}") | ||
|
|
||
| try: | ||
| editor.save_image() | ||
| print("FAIL: save_image did NOT raise RuntimeError") | ||
| except RuntimeError as e: | ||
| print(f"PASS: Caught RuntimeError: {e}") | ||
| except Exception as e: | ||
| print(f"FAIL: Caught unexpected exception: {type(e)} {e}") |
There was a problem hiding this comment.
Test is broken: no assertions and missing original_image setup.
Multiple issues make this test ineffective:
- No assertions — the test uses
print("PASS")/print("FAIL")instead ofself.assertRaises(RuntimeError, editor.save_image). The test will always pass regardless of behavior. editor.original_imageis never set —save_imagechecksif self.current_filepath is None or self.original_image is None: return None(editor.py line 1903-1904), so it exits early before reaching any save logic.- Wrong patch target —
PIL.Image.fromarrayshould be patched where it's looked up:faststack.imaging.editor.Image.fromarray, not at the PIL module level (though this is moot given issue#2). - Debug
printstatements should be removed for a committed test.
Proposed fix
class TestDebugError(unittest.TestCase):
def test_debug_save_image(self):
editor = ImageEditor()
editor.float_image = np.zeros((10, 10, 3), dtype=np.float32)
editor.current_filepath = Path("fake_path.jpg")
+ editor.original_image = MagicMock() # Required to pass early guard
- # Patch create_backup_file to succeed
with patch("faststack.imaging.editor.create_backup_file", return_value=Path("backup.jpg")):
- # Patch Image.fromarray to return a mock that fails to save
mock_img = MagicMock()
mock_img.save.side_effect = PermissionError("Mocked save error")
-
- print(f"DEBUG: Real Image.fromarray before patch: {Image.fromarray}")
- with patch("PIL.Image.fromarray", return_value=mock_img) as mock_fromarray:
- print(f"DEBUG: Image.fromarray is patched: {Image.fromarray}")
- print(f"DEBUG: mock_fromarray: {mock_fromarray}")
-
- # Verify that calling Image.fromarray returns our mock
- img = Image.fromarray(np.zeros((10,10,3), dtype=np.uint8))
- print(f"DEBUG: Returned img: {img}")
- print(f"DEBUG: img.save side effect: {img.save.side_effect}")
-
- try:
- editor.save_image()
- print("FAIL: save_image did NOT raise RuntimeError")
- except RuntimeError as e:
- print(f"PASS: Caught RuntimeError: {e}")
- except Exception as e:
- print(f"FAIL: Caught unexpected exception: {type(e)} {e}")
+ with patch("faststack.imaging.editor.Image.fromarray", return_value=mock_img):
+ with self.assertRaises(RuntimeError):
+ editor.save_image()🧰 Tools
🪛 Ruff (0.15.0)
[warning] 40-40: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@faststack/tests/debug_editor_error.py` around lines 12 - 41, The test
TestDebugError.test_debug_save_image is ineffective: set up
editor.original_image (e.g., assign a PIL Image or MagicMock) so save_image
proceeds past the early-return, patch Image.fromarray at the lookup site
faststack.imaging.editor.Image.fromarray (not PIL.Image.fromarray), keep the
create_backup_file patch and have the mocked image's save raise PermissionError,
remove all debug print statements, and replace prints with a proper assertion
using self.assertRaises(RuntimeError, editor.save_image) (or with
self.assertRaises in a context manager) to verify save_image raises the expected
RuntimeError.
| image_files = [MagicMock(path=Path(f"test_{i}.jpg")) for i in range(10)] | ||
| for img in image_files: | ||
| img.path.suffix = ".jpg" |
There was a problem hiding this comment.
Path.suffix is a read-only property — line 20 will raise AttributeError.
img.path is a real Path object (from MagicMock(path=Path(...))), and Path.suffix has no setter. This line will crash at test setup time. It's also redundant since Path("test_0.jpg").suffix is already ".jpg".
🐛 Proposed fix — remove the redundant line
image_files = [MagicMock(path=Path(f"test_{i}.jpg")) for i in range(10)]
- for img in image_files:
- img.path.suffix = ".jpg"🤖 Prompt for AI Agents
In `@faststack/tests/repro_futures_cleanup.py` around lines 18 - 20, The test
setup erroneously assigns to the read-only Path.suffix in the loop over
image_files (created via MagicMock(path=Path(...))), which will raise
AttributeError; remove the for-loop that does "for img in image_files:
img.path.suffix = '.jpg'" (or replace it by making img.path a MagicMock if you
really need assignable attributes), leaving image_files =
[MagicMock(path=Path(f"test_{i}.jpg")) for i in range(10)] as the correct setup
for tests that expect ".jpg" suffix.
| # COPIED LOGIC FROM app.py _handle_delete_failures | ||
| def _handle_delete_failures(self, result: DeleteResult, job: DeleteJob) -> None: | ||
| """Handle items that failed to delete. Rollback UI or prompt for perm delete.""" | ||
| if not result.failures: | ||
| return | ||
|
|
||
| # Identify which UI items failed (map back using paths) | ||
| # Note: We use the _key() mapping to ensure we match robustly | ||
| failed_keys = {self._key(f.jpg) for f in result.failures if f.jpg} | ||
|
|
||
| failed_indices_and_imgs = [] | ||
| for idx, img in job.removed_items: | ||
| if self._key(img.path) in failed_keys: | ||
| failed_indices_and_imgs.append((idx, img)) | ||
|
|
||
| if not failed_indices_and_imgs: | ||
| return | ||
|
|
||
| # Check if we should offer permanent delete (recycle bin error) | ||
| perm_candidates = [] # List of (idx, img) | ||
|
|
||
| # Helper to find if a specific failure code warrants perm delete | ||
| recycle_codes = { | ||
| DeletionErrorCodes.RECYCLE_FAILED.value, | ||
| DeletionErrorCodes.PERMISSION_DENIED.value, | ||
| DeletionErrorCodes.TRASH_FULL.value | ||
| } | ||
|
|
||
| # Map failure code by key for easy lookup | ||
| failure_map = {self._key(f.jpg): f for f in result.failures if f.jpg} | ||
|
|
||
| for idx, img in failed_indices_and_imgs: | ||
| f = failure_map.get(self._key(img.path)) | ||
| if f and f.code in recycle_codes: | ||
| perm_candidates.append((idx, img)) | ||
|
|
||
| if perm_candidates: | ||
| # Prompt user for permanent delete | ||
|
|
||
| # 1. Rollback non-candidates first | ||
| candidate_keys = {self._key(img.path) for _, img in perm_candidates} | ||
| to_rollback = [(i, img) for i, img in failed_indices_and_imgs if self._key(img.path) not in candidate_keys] | ||
|
|
||
| if to_rollback: | ||
| self._rollback_ui_items(to_rollback, job) | ||
|
|
||
| # 2. Ask user | ||
| # candidate_imgs = [img for _, img in perm_candidates] | ||
|
|
||
| # Using global mocks here instead of real imports | ||
| # confirmed = confirm_permanent_delete(...) | ||
|
|
||
| # For test purpose, we just assert that we identified candidates correctly | ||
| pass | ||
|
|
||
| else: | ||
| # Just rollback everything | ||
| self._rollback_ui_items(failed_indices_and_imgs, job) | ||
|
|
||
| self._rebuild_path_to_index() | ||
| self.sync_ui_state() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Copied production logic will silently drift from AppController._handle_delete_failures.
This duplicates ~60 lines from app.py. If the real method is refactored, this copy won't break but will test stale logic. Consider binding the real method onto MockController (as test_handle_failures.py does with __get__) instead of maintaining a separate copy.
Suggested approach
- # COPIED LOGIC FROM app.py _handle_delete_failures
- def _handle_delete_failures(self, result: DeleteResult, job: DeleteJob) -> None:
- ... # 60 lines of copied code
+ # Bind the real method
+ from faststack.app import AppController
+ _handle_delete_failures = AppController._handle_delete_failuresYou would need to add a stub for confirm_permanent_delete / confirm_batch_permanent_delete via patching, similar to test_handle_failures.py.
🤖 Prompt for AI Agents
In `@faststack/tests/test_handle_failures_isolated.py` around lines 25 - 85, The
test copies production logic from AppController._handle_delete_failures into
MockController._handle_delete_failures causing duplication and potential drift;
instead bind the real method onto the mock like test_handle_failures.py does
using AppController._handle_delete_failures.__get__(mock, MockController) so the
test uses the production implementation, and patch any external UI prompts
(confirm_permanent_delete or confirm_batch_permanent_delete) via a stub/mock so
the method can run in tests without user interaction.
| @pytest.fixture | ||
| def model(tmp_path): | ||
| # Mocking dependencies that might trigger complex I/O or UI logic | ||
| with ( | ||
| patch('faststack.thumbnail_view.model.count_images_in_folder', return_value=0), | ||
| patch('faststack.thumbnail_view.model.read_folder_stats', return_value=None), | ||
| patch('faststack.thumbnail_view.model.find_images', return_value=[]), | ||
| ): | ||
| model = ThumbnailModel(tmp_path, tmp_path) | ||
| # Mock Qt-specific calls that need a running event loop or app | ||
| model.beginResetModel = Mock() | ||
| model.endResetModel = Mock() | ||
| model.selectionChanged = Mock() | ||
| return model |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file and explore the structure
find . -name "test_refresh_crash.py" -type f 2>/dev/null | head -5Repository: AlanRockefeller/faststack
Length of output: 108
🏁 Script executed:
# Also locate the ThumbnailModel class
find . -path "*/thumbnail_view/model.py" -type f 2>/dev/nullRepository: AlanRockefeller/faststack
Length of output: 104
🏁 Script executed:
# Get a general view of the repository structure
git ls-files | grep -E "(thumbnail|test_refresh)" | head -20Repository: AlanRockefeller/faststack
Length of output: 743
🏁 Script executed:
# Read the test file to understand what tests do
cat -n ./faststack/tests/test_refresh_crash.pyRepository: AlanRockefeller/faststack
Length of output: 1454
🏁 Script executed:
# Read the ThumbnailModel class to check refresh() implementation
wc -l ./faststack/thumbnail_view/model.pyRepository: AlanRockefeller/faststack
Length of output: 108
🏁 Script executed:
# Read the refresh() method implementation
sed -n '1,765p' ./faststack/thumbnail_view/model.py | grep -n "def refresh" -A 50 | head -80Repository: AlanRockefeller/faststack
Length of output: 3493
🏁 Script executed:
# Also check the imports and beginning of the file
head -50 ./faststack/thumbnail_view/model.pyRepository: AlanRockefeller/faststack
Length of output: 1209
🏁 Script executed:
# Check if find_images can handle empty tmp_path gracefully
rg -n "def find_images" -A 20 ./faststack/io/indexer.py | head -40Repository: AlanRockefeller/faststack
Length of output: 1043
🏁 Script executed:
# Check other test fixtures in the same test directory for comparison
grep -n "yield\|return" ./faststack/tests/test_refresh_optimization.py -B 5 -A 2 | head -50Repository: AlanRockefeller/faststack
Length of output: 533
🏁 Script executed:
# Verify: Does find_images work with an empty directory?
# Read the full find_images function to understand error handling
sed -n '24,100p' ./faststack/io/indexer.pyRepository: AlanRockefeller/faststack
Length of output: 3202
🏁 Script executed:
# Check what actually happens in refresh() - does it use t0, t1, t2, t3?
sed -n '352,410p' ./faststack/thumbnail_view/model.pyRepository: AlanRockefeller/faststack
Length of output: 2305
🏁 Script executed:
# Also check _add_folders_to_entries to see if it calls patched functions
rg -n "_add_folders_to_entries" ./faststack/thumbnail_view/model.py -A 20 | head -40Repository: AlanRockefeller/faststack
Length of output: 1791
🏁 Script executed:
# Check if _add_folders_to_entries calls count_images_in_folder or read_folder_stats
sed -n '306,350p' ./faststack/thumbnail_view/model.pyRepository: AlanRockefeller/faststack
Length of output: 1846
🏁 Script executed:
# Verify the full code path - check if count_images_in_folder is called anywhere in refresh
rg -n "count_images_in_folder\|read_folder_stats" ./faststack/thumbnail_view/model.pyRepository: AlanRockefeller/faststack
Length of output: 51
Patches are only active during model construction, not during test execution.
The with block exits before the fixture returns, so find_images, count_images_in_folder, and read_folder_stats are unpatched when tests call model.refresh() or model.refresh_from_controller(). Both methods invoke these functions—specifically, _add_folders_to_entries() calls read_folder_stats() and count_images_in_folder(), and refresh() calls find_images()—meaning tests execute against real implementations, not mocks, making the NameError regression check unreliable.
Use yield instead of return to keep patches active throughout test execution:
Proposed fix
`@pytest.fixture`
def model(tmp_path):
- # Mocking dependencies that might trigger complex I/O or UI logic
with (
patch('faststack.thumbnail_view.model.count_images_in_folder', return_value=0),
patch('faststack.thumbnail_view.model.read_folder_stats', return_value=None),
patch('faststack.thumbnail_view.model.find_images', return_value=[]),
):
model = ThumbnailModel(tmp_path, tmp_path)
- # Mock Qt-specific calls that need a running event loop or app
model.beginResetModel = Mock()
model.endResetModel = Mock()
model.selectionChanged = Mock()
- return model
+ yield model🤖 Prompt for AI Agents
In `@faststack/tests/test_refresh_crash.py` around lines 6 - 19, The patches in
the model fixture currently exit before tests run because the with-block ends
prior to returning the fixture; change the fixture so the patch context stays
active for the test run by yielding the model from inside the patch context (or
by creating patchers with patch(...) and starting them before constructing the
ThumbnailModel and stopping them after yielding). Specifically update the
fixture named model so the patch context that wraps
patch('faststack.thumbnail_view.model.find_images'),
patch('faststack.thumbnail_view.model.count_images_in_folder'), and
patch('faststack.thumbnail_view.model.read_folder_stats') remains active across
the yield, ensuring calls from ThumbnailModel.refresh(),
refresh_from_controller(), and _add_folders_to_entries() use the mocks during
test execution.
| @pytest.fixture | ||
| def thumbnail_model(): | ||
| """Create a ThumbnailModel with fake entries for testing.""" | ||
| from faststack.thumbnail_view.model import ThumbnailModel, ThumbnailEntry | ||
|
|
||
| model = ThumbnailModel( | ||
| base_directory=Path("/fake/dir"), | ||
| current_directory=Path("/fake/dir"), | ||
| thumbnail_size=200, | ||
| ) | ||
|
|
||
| # Manually add entries (bypass refresh which would scan disk) | ||
| entries = [ | ||
| ThumbnailEntry(path=Path("/fake/dir/img001.jpg"), name="img001.jpg", is_folder=False, mtime_ns=1000), | ||
| ThumbnailEntry(path=Path("/fake/dir/img002.jpg"), name="img002.jpg", is_folder=False, mtime_ns=2000), | ||
| ThumbnailEntry(path=Path("/fake/dir/img003.jpg"), name="img003.jpg", is_folder=False, mtime_ns=3000), | ||
| ] | ||
|
|
||
| model.beginResetModel() | ||
| model._entries = entries | ||
| model._rebuild_id_mapping() | ||
| model.endResetModel() | ||
|
|
||
| return model |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ThumbnailModel or any pytest conftest sets up a QApplication automatically
echo "=== Check ThumbnailModel base class ==="
rg -n 'class ThumbnailModel' --type=py -A3
echo ""
echo "=== Check for qapp fixture or QCoreApplication in conftest ==="
fd conftest.py --exec cat {}
echo ""
echo "=== Check for pytest-qt in requirements ==="
fd requirements -e txt --exec grep -i 'pytest-qt' {}
fd pyproject.toml --exec grep -i 'pytest-qt' {}
fd setup.py --exec grep -i 'pytest-qt' {}
fd setup.cfg --exec grep -i 'pytest-qt' {}Repository: AlanRockefeller/faststack
Length of output: 1452
🏁 Script executed:
#!/bin/bash
# Check the actual test file for qapp fixture or QCoreApplication setup
echo "=== Check test_thumbnail_ready_emits_datachanged.py for QCoreApplication ==="
cat faststack/tests/test_thumbnail_ready_emits_datachanged.py
echo ""
echo "=== Check other test files for qapp fixture pattern ==="
grep -r 'qapp\|QCoreApplication\|QApplication' faststack/tests/*.py | head -20
echo ""
echo "=== Check for pytest plugins ==="
fd pyproject.toml --exec cat {}
</parameter>Repository: AlanRockefeller/faststack
Length of output: 6736
🏁 Script executed:
#!/bin/bash
# Check ThumbnailModel constructor to see if it requires QApplication
echo "=== Check ThumbnailModel constructor ==="
sed -n '87,150p' faststack/thumbnail_view/model.py
echo ""
echo "=== Check if test_recycle_bin_tracking.py has qapp setup ==="
grep -n 'QApplication\|qapp\|@pytest.fixture' faststack/tests/test_recycle_bin_tracking.py | head -20
</parameter>Repository: AlanRockefeller/faststack
Length of output: 3167
🏁 Script executed:
#!/bin/bash
# Check if test_recycle_bin_tracking.py has qapp fixture
echo "=== Check test_recycle_bin_tracking.py for QApplication setup ==="
cat faststack/tests/test_recycle_bin_tracking.py | head -50
echo ""
echo "=== Check if any conftest sets up pytest fixtures ==="
cat faststack/tests/conftest.py
echo ""
echo "=== Search for qapp fixture in all test files ==="
grep -r '@pytest.fixture' faststack/tests/ --include='*.py' -A2 | grep -B2 'qapp'Repository: AlanRockefeller/faststack
Length of output: 3769
Add QCoreApplication fixture to ensure Qt signal initialization works.
The test file creates a ThumbnailModel (which inherits from QAbstractListModel and connects signals in __init__) without setting up a QCoreApplication. While other test files in the suite (e.g., test_recycle_bin_tracking.py) explicitly create a QCoreApplication before instantiating Qt objects, this test does not.
Add a session-scoped qapp fixture (as shown in test_recycle_bin_tracking.py and other test files) to ensure Qt signals can be properly initialized:
`@pytest.fixture`(scope="session")
def qapp():
"""Ensure a QCoreApplication exists for Qt signal/slot connections."""
from PySide6.QtCore import QCoreApplication
app = QCoreApplication.instance()
if not app:
app = QCoreApplication([])
return appThen add qapp as a parameter to the thumbnail_model fixture (e.g., def thumbnail_model(qapp):).
🤖 Prompt for AI Agents
In `@faststack/tests/test_thumbnail_ready_emits_datachanged.py` around lines 12 -
35, The test creates a ThumbnailModel (which subclasses QAbstractListModel)
before a Qt application exists, so add a session-scoped qapp fixture that
ensures a QCoreApplication instance is available (create one if
QCoreApplication.instance() is None) and then accept qapp as a parameter to the
existing thumbnail_model fixture (change def thumbnail_model(): to def
thumbnail_model(qapp):) so ThumbnailModel initialization and its signal/slot
connections occur with a valid QCoreApplication; reference ThumbnailModel,
thumbnail_model fixture, and QCoreApplication/qapp when making the edits.
| if self._active_filter_flags and self._get_metadata: | ||
| flags = self._active_filter_flags | ||
| filtered = [] | ||
| for img in images: | ||
| try: | ||
| meta = self._get_metadata(img.path.stem) | ||
| is_stacked = meta.get("stacked", False) | ||
| is_uploaded = meta.get("uploaded", False) | ||
| is_edited = meta.get("edited", False) | ||
| is_restacked = meta.get("restacked", False) | ||
| if all(meta.get(flag, False) for flag in flags): | ||
| filtered.append(img) | ||
| except Exception: | ||
| pass | ||
|
|
||
| self._entries.append( | ||
| ThumbnailEntry( | ||
| path=img.path, | ||
| name=img.path.name, | ||
| is_folder=False, | ||
| is_stacked=is_stacked, | ||
| is_uploaded=is_uploaded, | ||
| is_edited=is_edited, | ||
| is_restacked=is_restacked, | ||
| mtime_ns=mtime_ns, | ||
| ) | ||
| ) | ||
| pass # Skip images with metadata errors | ||
| images = filtered |
There was a problem hiding this comment.
Flag filtering silently drops images when metadata callback returns non-dict objects.
meta.get(flag, False) at Line 384 assumes meta is a dict. If _get_metadata returns an EntryMetadata-like object (as the test model's _normalize_meta_flags accounts for), .get() will raise AttributeError, the bare except catches it, and the image is silently excluded. All flagged images would vanish from filtered results.
The same pattern occurs in refresh_from_controller (Lines 498-509) and _add_images_to_entries (Lines 553-561).
Consider normalizing metadata consistently (as the test model does with _normalize_meta_flags) or at minimum logging a warning:
Suggested improvement for the except block
except Exception:
- pass # Skip images with metadata errors
+ log.debug("Skipping image %s due to metadata error", img.path.stem, exc_info=True)🧰 Tools
🪛 Ruff (0.15.0)
[error] 386-387: try-except-pass detected, consider logging the exception
(S110)
[warning] 386-386: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@faststack/thumbnail_view/model.py` around lines 378 - 388, The filtering
currently assumes the metadata returned by _get_metadata is a dict and uses
meta.get(...), which causes AttributeError and silently drops images; update the
filter logic in the block that uses self._active_filter_flags and
self._get_metadata to first normalize the metadata (reuse the existing
_normalize_meta_flags helper) or coerce it to a mapping-like dict before calling
.get, and replace the bare except: pass with a warning log (don’t swallow
errors) so images aren’t silently excluded; apply the same fix patterns to the
similar code paths in refresh_from_controller and _add_images_to_entries.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance