Refine grid view and fix lots of bugs#37
Conversation
WalkthroughAdds recycle-bin and permanent-delete utilities, folder statistics with coverage buckets and caching, expanded thumbnail/model metadata and navigation, QML grid and tile cursor/selection UX, new UIState signals/properties and keybindings, improvements to pairing/indexing, plus many formatting/test updates. Changes
Sequence DiagramssequenceDiagram
participant User as User
participant MainWindow as Main Window
participant UIState as UIState
participant AppController as App Controller
participant FileSystem as File System
participant Sidecar as Sidecar Manager
User->>MainWindow: Trigger Delete (Recycle)
MainWindow->>AppController: request_delete(image)
AppController->>FileSystem: move files → recycle bin
FileSystem-->>AppController: move result
AppController->>Sidecar: record undo entry
AppController->>UIState: update active_recycle_bins
UIState-->>MainWindow: hasRecycleBinItems changed
MainWindow-->>User: show confirmation / UI update
sequenceDiagram
participant User as User
participant MainWindow as Main Window
participant UIState as UIState
participant Dialog as Recycle Dialog
participant AppController as App Controller
participant FileSystem as File System
User->>MainWindow: Close window
MainWindow->>UIState: hasRecycleBinItems?
UIState-->>MainWindow: true
MainWindow->>Dialog: show recycleBinCleanupDialog
alt User confirms cleanup
Dialog->>AppController: cleanup_recycle_bins()
AppController->>FileSystem: permanently delete bins
FileSystem-->>AppController: deleted
AppController->>UIState: clear active_recycle_bins
Dialog-->>MainWindow: allow close
else User cancels
Dialog-->>MainWindow: block/keep files
end
sequenceDiagram
participant User as User
participant Grid as Thumbnail Grid
participant Model as Thumbnail Model
participant FolderStats as Folder Stats
participant Prefetcher as Prefetcher
User->>Grid: navigate_to(path)
Grid->>Model: navigate_to(path)
Model->>FolderStats: read_folder_stats(path)
FolderStats-->>Model: FolderStats(jpg_count, raw_count, buckets)
Model-->>Grid: refresh entries with folder_stats
Grid->>Prefetcher: adjust prefetch window
Prefetcher-->>Grid: decode/cache thumbnails
Grid-->>User: display grid with overlays
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
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 (3)
faststack/tests/test_editor_error_handling.py (1)
14-31: Patch order and target forcv2will not affect the editor module due to early binding inoptional_deps.
faststack.imaging.editorimportscv2fromfaststack.imaging.optional_depsat module scope (line 30), which itself performs a try/except import ofcv2at module scope (optional_deps.py lines 3–9). SinceImageEditoris imported beforepatch.dict(sys.modules, {"cv2": ...})is applied, thecv2reference inoptional_depsis already bound and won't be affected by latersys.modulespatches.When
load_imagecallscv2.imread()(line ~454), it uses the already-bound reference fromoptional_deps, not the patchedsys.modules["cv2"].The
PIL.Image.openpatch target is correct sinceImageis imported directly at module scope and used asImage.open().Apply
patch.dict(sys.modules, {"cv2": MagicMock()})before importingImageEditor, or patch the module-scoped binding directly atfaststack.imaging.optional_deps.cv2:🔧 Recommended fix
- from faststack.imaging.editor import ImageEditor - editor = ImageEditor() - - with patch("PIL.Image.open", side_effect=OSError("Mocked file error")): - with patch.dict(sys.modules, {"cv2": MagicMock()}): - sys.modules["cv2"].imread.return_value = None - result = editor.load_image("non_existent_file.jpg") - self.assertFalse(result) + with patch.dict(sys.modules, {"cv2": MagicMock()}): + sys.modules["cv2"].imread.return_value = None + from faststack.imaging.editor import ImageEditor + editor = ImageEditor() + with patch("PIL.Image.open", side_effect=OSError("Mocked file error")): + result = editor.load_image("non_existent_file.jpg") + self.assertFalse(result)faststack/tests/test_highlight_state_normalization.py (1)
16-34: Fix duplicate test name and make the legacy-key test assertive.Line 16 redefines the same test name used on Line 34, so the first test is overwritten and never runs. Rename it and add assertions so it enforces the legacy-key behavior.
Proposed fix
- def test_highlight_state_normalization_standard(self): - """Test with standard keys.""" + def test_highlight_state_normalization_legacy_keys(self): + """Test with legacy keys.""" self.mock_editor._last_highlight_state = { "headroom_pct": 0.1, "clipped_pct": 0.2, "near_white_pct": 0.3, } - # Controller returns canonical keys using the passed dict (even if they were wrong in backend, provider normalizes? + state = self.ui_state.highlightState + self.assertEqual(state["headroom_pct"], 0.1) + self.assertEqual(state["source_clipped_pct"], 0.0) + self.assertEqual(state["current_nearwhite_pct"], 0.0)faststack/io/watcher.py (1)
76-78: Return a strict bool fromis_alive().The expression
self.observer and self.observer.is_alive()returnsNonewhenself.observerisNone, violating the-> booltype annotation. Usebool()to ensure the return is always a strict bool.Suggested fix
- return self.observer and self.observer.is_alive() + return bool(self.observer and self.observer.is_alive())
🤖 Fix all issues with AI agents
In `@faststack/tests/check_imports.py`:
- Around line 7-23: Replace the no-op print-only try blocks in check_imports.py
with real imports of the modules (import faststack.app and import
faststack.tests.test_raw_pipeline) and narrow the except clauses to catch
ImportError instead of Exception; in each except block, log the import error and
print the traceback (use traceback.print_exc()) so failures are reported with
details. Ensure the import statements are placed inside the try blocks that
currently reference those modules so the script actually verifies importability.
In `@faststack/tests/test_permanent_delete.py`:
- Around line 14-64: Replace the inlined mock helper functions with tests that
invoke the real AppController methods: instantiate AppController, set its
self.recycle_bin_dir to the tmp_path value before calling
_ensure_recycle_bin_dir(), and assert it returns True/False and that the
directory exists or not; for _confirm_permanent_delete(), patch QMessageBox (or
monkeypatch AppController._show_confirm_dialog) to simulate user clicking
confirm/cancel and call AppController._confirm_permanent_delete(mock_img) to
assert returned bool; if testing _permanently_delete_image_files(), create temp
files, set AppController.recycle_bin_dir, and patch logging/OS operations as
needed to assert correct moves, deletions, and logged errors. Use the actual
method names (_ensure_recycle_bin_dir, _confirm_permanent_delete,
_permanently_delete_image_files), the AppController class, and patching of
QMessageBox/self.recycle_bin_dir to avoid GUI and environment side effects.
In `@faststack/ui/provider.py`:
- Around line 1348-1373: The properties and method (recycleBinStatsText,
hasRecycleBinItems, cleanupRecycleBins) reference self._app_controller which
doesn't exist because the controller is stored as self.app_controller; change
those references to self.app_controller so get_recycle_bin_stats() and
cleanup_recycle_bins() are called on the correct attribute and avoid
AttributeError.
🟡 Minor comments (20)
faststack/tests/test_editor_error_handling.py-48-55 (1)
48-55: Fix patch target for module-scopedImagereference.The
faststack.imaging.editormodule imports Image directly withfrom PIL import Image(line 9), soImage.fromarray()calls use a module-scoped reference. PatchingPIL.Image.fromarraywon't intercept this; patchfaststack.imaging.editor.Image.fromarrayinstead.Fix
- with patch("PIL.Image.fromarray", return_value=mock_img): + with patch("faststack.imaging.editor.Image.fromarray", return_value=mock_img): with self.assertRaises(RuntimeError) as cm: editor.save_image()faststack/tests/reproduce_exif_bug.py-25-44 (1)
25-44: Clarify docstrings in this reproduction script—they contradict the intent.This file won't be picked up by pytest (doesn't match the
test_*.pydiscovery pattern inpyproject.toml), so it remains a standalone reproduction script. However, the docstrings are misleading: they claim the method "currently drops EXIF data," but the test assertions and inline comments explicitly state "DESIRED BEHAVIOR: It returns the original bytes."The actual implementation in
editor.py(_get_sanitized_exif_bytes, lines 1600–1630) returnsNoneon tobytes failure or missing, which aligns with the assertions intest_exif_compat.py(lines 63–77).Update the docstrings to remove "currently" or clarify that this reproduces the desired future behavior, not the current behavior. For example, change "Verify that a failure in tobytes() currently drops EXIF data" to "Verify that a failure in tobytes() should return original bytes (desired behavior)."
faststack/test_pil_blur.py-6-29 (1)
6-29: Don't swallow exceptions in test code.The broad
Exceptioncatch only prints to stdout, masking failures. When this test runs (either manually or if pytest configuration changes), failures will silently pass. Either re-raise the exception or use a test assertion.Proposed fix
except Exception as e: print(f"Failed: {e}") + raisefaststack/tests/test_executable_validator.py-64-68 (1)
64-68: Prefix unusederrorwith_to avoid lint errors.
erroris never used in the test functions. Rename to_error(or_) to satisfyRUF059:
- Line 64 in
test_suspicious_path_with_traversal()- Line 127 in
test_wrong_executable_name_for_type()Proposed fix
- is_valid, error = validate_executable_path(suspicious_path) + is_valid, _error = validate_executable_path(suspicious_path) ... - is_valid, error = validate_executable_path(wrong_exe, app_type="photoshop") + is_valid, _error = validate_executable_path(wrong_exe, app_type="photoshop")faststack/tests/test_executable_validator.py-33-40 (1)
33-40: Fix unused lambda arguments to satisfy Ruff (ARG005).Rename unused
selfparameter to_selfin three lambda assignments to match Ruff's dummy variable pattern and avoid theunused-lambda-argumentviolation.Proposed fix
- mock_path_instance.__str__ = lambda self: photoshop_path + mock_path_instance.__str__ = lambda _self: photoshop_path- mock_path_instance.__str__ = lambda self: r"C:\Windows\System32\malware.exe" + mock_path_instance.__str__ = lambda _self: r"C:\Windows\System32\malware.exe"- mock_path_instance.__str__ = lambda self: wrong_exe + mock_path_instance.__str__ = lambda _self: wrong_exeAlso applies to lines 54 and 117.
faststack/tests/mini_test.py-27-40 (1)
27-40: Re-raise exceptions in both test blocks for consistency and visibility.The inner
except Exception as eblocks (lines 20 and 39-40) only print without re-raising. This masks unexpected failures in manual test runs. Additionally, the same pattern appears in Test 1 (lines 19-20) but the suggestion is missing there.To improve code robustness and consistency:
- Test 1 (lines 19-20): Add
raiseafter the print- Test 2 (lines 39-40): Add
raiseafter the printWhen re-raised, unexpected exceptions will propagate to the outer exception handler (lines 45-46) and be clearly reported as "CRASH: {e}", making test failures more visible.
faststack/tests/test_drag_logic.py-7-9 (1)
7-9: Avoid unused parameter to keep tests lint-clean.
current_indexis unused on Line 7. If it’s only for signature parity, prefix with_to satisfy Ruff and keep intent clear.Proposed fix
-def get_drag_paths( - image_files, current_index, existing_indices, current_edit_source_mode -): +def get_drag_paths( + image_files, _current_index, existing_indices, current_edit_source_mode +):faststack/imaging/math_utils.py-80-84 (1)
80-84: Replace the multiplication sign in the docstring to satisfy Ruff.Line 83 contains
H×W, which triggers RUF002 in Ruff.Proposed fix
- srgb_u8: Optional uint8 sRGB array (source image) for accurate JPEG clipping detection. - MUST have same H×W dimensions as rgb_linear (or be stride-compatible). + srgb_u8: Optional uint8 sRGB array (source image) for accurate JPEG clipping detection. + MUST have same H x W dimensions as rgb_linear (or be stride-compatible).faststack/tests/test_raw_pipeline.py-33-37 (1)
33-37: Silence unused*args/**kwargsin test helper callbacks.
These are flagged by Ruff; rename to_args/_kwargs.🧹 Proposed fix
- def side_effect_start(*args, **kwargs): + def side_effect_start(*_args, **_kwargs): _, thread_kwargs = mock_thread.call_args target = thread_kwargs.get("target") if target: target()- def side_effect_start(*args, **kwargs): + def side_effect_start(*_args, **_kwargs): _, thread_kwargs = mock_thread.call_args target = thread_kwargs.get("target") if target: target()- def side_effect_start(*args, **kwargs): + def side_effect_start(*_args, **_kwargs): _, thread_kwargs = mock_thread.call_args target = thread_kwargs.get("target") if target: target()Also applies to: 78-83, 131-136
faststack/tests/test_raw_pipeline.py-260-263 (1)
260-263: Avoid unusedbackup_path.
Rename to_backup_pathto satisfy RUF059.🧹 Proposed fix
- saved_path, backup_path = res + saved_path, _backup_path = resfaststack/tests/test_editor_lifecycle_and_safety.py-41-44 (1)
41-44: Drop unusedmock_editor_clsbinding.
Ruff flags this as unused; the patch context works without binding.🧹 Proposed fix
- with patch("faststack.app.ImageEditor") as mock_editor_cls: + with patch("faststack.app.ImageEditor"): self.controller = AppController(Path("."), self.mock_engine) self.mock_editor_instance = self.controller.image_editorfaststack/tests/test_raw_pipeline.py-212-229 (1)
212-229: Rename unusedmock_runparameter.
Ruff flags it; the patch is still needed, so rename to_mock_run.🧹 Proposed fix
- def test_develop_raw_slot(self, mock_config_get, mock_run, mock_exists): + def test_develop_raw_slot(self, mock_config_get, _mock_run, mock_exists):faststack/tests/test_generation_aware_preview.py-50-52 (1)
50-52: Remove unusedimgassignment.
This triggers Ruff F841 and the initial call is redundant.🧹 Proposed fix
- img = self.provider.requestImage("0/5", None, None)faststack/tests/test_reactive_delete.py-35-36 (1)
35-36: Remove unusedtmp_pathparameters (Ruff ARG001).They’re not referenced in either test.
♻️ Proposed cleanup
-def test_reactive_delete_fallback(app_controller, tmp_path): +def test_reactive_delete_fallback(app_controller): @@ -def test_reactive_delete_fallback_cancelled(app_controller, tmp_path): +def test_reactive_delete_fallback_cancelled(app_controller):Also applies to: 72-73
faststack/tests/test_config_setters.py-14-33 (1)
14-33: Silence Ruff unused-argument warnings in mocks.
name,args, andkwargsare unused and flagged by Ruff. Prefix with_to keep signatures but avoid lint noise.🧹 Suggested fix
class MockQObject: - def property(self, name): + def property(self, _name): return None - def setProperty(self, name, value): + def setProperty(self, _name, _value): pass @@ -def MockSlot(*args, **kwargs): +def MockSlot(*_args, **_kwargs): def decorator(func): return funcfaststack/io/sidecar.py-13-34 (1)
13-34: Narrow the broadexcept Exceptionin metadata parsing.Catching all exceptions can hide programming errors and is flagged by Ruff (BLE001). Consider validating input type and catching more specific exceptions.
🛡️ Suggested change
def _entrymetadata_from_json(meta: dict) -> EntryMetadata: @@ - try: + if not isinstance(meta, dict): + log.warning("Error parsing metadata entry: expected dict, got %s", type(meta).__name__) + return EntryMetadata() + try: @@ - except Exception as e: + except (TypeError, ValueError) as e: log.warning(f"Error parsing metadata entry: {e}") return EntryMetadata()faststack/tests/test_config_setters.py-109-113 (1)
109-113: Remove unusedmock_path_clsassignment.It’s unused and flagged by Ruff. Dropping it keeps the setup clean.
🧹 Suggested fix
- mock_path_cls = self.patches[ - -1 - ].target # access the mock object ? NO, p.start returns mock - # Ideally capture the return of start() - # Simpler: just instantiate. The mocks are active.faststack/ui/provider.py-139-141 (1)
139-141: Duplicate signal declaration.
preloadingStateChangedis declared twice on lines 139 and 140. Remove the duplicate.🐛 Proposed fix
preloadingStateChanged = Signal() - preloadingStateChanged = Signal() preloadProgressChanged = Signal()faststack/tests/thumbnail_view/test_folder_stats.py-235-243 (1)
235-243: Avoid the unused variable to keep ruff clean.
jpg_filesis unused in this test, which triggers RUF059.♻️ Suggested tweak
- jpg_count, raw_count, jpg_files = _scan_folder_files(temp_folder) + jpg_count, raw_count, _jpg_files = _scan_folder_files(temp_folder)faststack/qml/ThumbnailGridView.qml-139-200 (1)
139-200: Add guards for index-sensitive actions to prevent unnecessary backend calls with stale indices.When the model shrinks (e.g., after deletion),
currentIndexcan remain stale and exceed the new item count. While the backend methods safely handle out-of-range indices by checking bounds and returning early, guarding at the QML layer prevents unnecessary calls and keeps the code flow clear.🛡️ Suggested guard for index-sensitive actions
} else if (event.key === Qt.Key_Return || event.key === Qt.Key_Enter) { - uiState.gridOpenIndex(thumbnailGrid.currentIndex) + if (thumbnailGrid.currentIndex >= 0 && thumbnailGrid.currentIndex < thumbnailGrid.count) { + uiState.gridOpenIndex(thumbnailGrid.currentIndex) + } event.accepted = true } else if (event.key === Qt.Key_Space) { - uiState.gridSelectIndex(thumbnailGrid.currentIndex, false, true) + if (thumbnailGrid.currentIndex >= 0 && thumbnailGrid.currentIndex < thumbnailGrid.count) { + uiState.gridSelectIndex(thumbnailGrid.currentIndex, false, true) + } event.accepted = true } else if (event.key === Qt.Key_Delete || event.key === Qt.Key_Backspace) { - uiState.gridDeleteAtCursor(thumbnailGrid.currentIndex) + if (thumbnailGrid.currentIndex >= 0 && thumbnailGrid.currentIndex < thumbnailGrid.count) { + uiState.gridDeleteAtCursor(thumbnailGrid.currentIndex) + } event.accepted = true }
🧹 Nitpick comments (27)
faststack/tests/debug_metadata.py (1)
58-62: Broad exception catch is acceptable for this debug utility.The static analysis tool flags
BLE001here, but this is a debug script where catching all exceptions is intentional—the purpose is to capture any failure and write the full traceback to a file for analysis. The production code infaststack/imaging/metadata.pyuses the same pattern with an explicit# noqa: BLE001comment.If you want to silence the linter, consider adding the same noqa directive:
- except Exception: + except Exception: # noqa: BLE001 - debug script captures all failuresfaststack/tests/test_sidecar.py (1)
16-16: Consider fixing the type hint for PEP 484 compliance.The parameter
content: dict = Noneuses an implicitOptional, which PEP 484 prohibits. Modern type checkers expect an explicit union withNone.🔧 Proposed fix
- def _create(content: dict = None): + def _create(content: dict | None = None):As per coding guidelines: Static analysis hint from Ruff (RUF013) - "PEP 484 prohibits implicit
Optional".faststack/tests/manual_test_error_handling.py (1)
23-38: Consider documenting the expected exception flow.The test expects
load_imageto raiseOSErrorwhenPIL.Image.openfails, but Line 26 returnsTrueinside theexcept OSErrorblock. While this works correctly, the static analysis hint (TRY300) suggests moving success returns to anelseblock for clarity. This is a minor style preference and not blocking.Optional: Use else block for success path
except OSError as e: if "Mocked file error" in str(e): - print("SUCCESS: load_image raised expected exception") - return True - else: - print(f"FAILURE: load_image raised wrong exception: {e}") - return False + pass # Expected exception + else: + print(f"FAILURE: load_image raised wrong exception: {e}") + return False + else: + print("FAILURE: load_image did NOT raise exception") + return False + print("SUCCESS: load_image raised expected exception") + return Truefaststack/tests/test_auto_levels.py (1)
22-22: Consider using_prefix for intentionally unused tuple elements.Static analysis flags
blacksandp_lowas unused in some tests. While understandable given you're only verifying specific return values, prefixing with_clarifies intent.Example for line 22
- blacks, whites, p_low, p_high = editor.auto_levels(threshold_percent=0.0) + _blacks, whites, p_low, p_high = editor.auto_levels(threshold_percent=0.0)Also applies to: 91-92
faststack/tests/test_cache_invalidation.py (2)
49-66: Consider converting print-based checks to proper assertions.The test uses
print("PASS/FAIL")patterns instead of assertions, which won't cause test failures when run by pytest. This could mask regressions.Example conversion
if hash1 == hash2: print("PASS: Hash is stable across reloads.") else: - print("FAIL: Hash changed across reloads (unnecessary invalidation).") + raise AssertionError("FAIL: Hash changed across reloads (unnecessary invalidation).")Or use
assert hash1 == hash2, "Hash changed across reloads".
69-72: Avoid bareexcept:clause.The bare
except:catches all exceptions includingKeyboardInterruptandSystemExit. Consider usingexcept Exception:orexcept OSError:for the cleanup.Proposed fix
try: shutil.rmtree(test_dir) - except: + except Exception: passfaststack/tests/test_new_features.py (2)
164-184: Consider using_prefix for intentionally unused variables.At line 174,
blacksandwhitesare unpacked but not used since this test case only validates the degenerate dynamic range behavior.Proposed fix
- blacks, whites, p_low, p_high = self.editor.auto_levels(threshold_percent) + _blacks, _whites, p_low, p_high = self.editor.auto_levels(threshold_percent)
241-257: Consider using_prefix for unused variables in monotonicity check.At line 248,
p_lowandp_highare not used in the subsequent monotonicity verification.Proposed fix
- blacks, whites, p_low, p_high = self.editor.auto_levels(threshold_percent) + blacks, whites, _p_low, _p_high = self.editor.auto_levels(threshold_percent)faststack/tests/test_prefetch_logic.py (1)
50-52: Use unittest assertions instead of raising a generic Exception.On Line 51,
raise Exception(...)should be an assertion for cleaner test output and to satisfy TRY002/TRY003.Proposed fix
- if 4 not in prefetcher.futures: - raise Exception("Task 4 was not added to futures!") + self.assertIn(4, prefetcher.futures, "Task 4 was not added to futures!")faststack/config.py (1)
145-152: Avoid unconditional config writes on every load.
self.save()runs even when nothing was added, which can cause unnecessary disk churn. Consider tracking whether defaults were inserted before saving.♻️ Suggested change
- for section, keys in DEFAULT_CONFIG.items(): + updated = False + for section, keys in DEFAULT_CONFIG.items(): if not self.config.has_section(section): self.config.add_section(section) + updated = True for key, value in keys.items(): if not self.config.has_option(section, key): self.config.set(section, key, value) - self.save() # Save to add any missing keys + updated = True + if updated: + self.save() # Save to add any missing keysfaststack/imaging/editor.py (1)
1625-1634: Consider narrowing the exception catch (optional).The broad
except Exceptioncatch at line 1627 is flagged by static analysis. While the defensive fallback toNoneis appropriate for EXIF handling (which can fail in many unpredictable ways), consider narrowing to more specific exception types if the failure modes are well-understood:except (OSError, ValueError, TypeError, AttributeError) as e:However, given that EXIF libraries can raise various unexpected exceptions from corrupt/malformed data, the current approach is pragmatic and the warning log provides visibility.
faststack/imaging/prefetch.py (2)
453-458: Broad exception catches are pragmatic for image decoding fallbacks.Static analysis flags multiple
except Exceptionblocks (lines 453, 473, 609, 687, 749, 762, 795). In the context of image decoding from arbitrary files, these broad catches are reasonable - corrupt images, unsupported formats, and I/O errors can manifest as many different exception types.The current pattern:
- Logs the specific error with file path
- Falls back gracefully (returns None or tries next decoder)
- Doesn't crash the prefetch worker thread
If you want to tighten this in the future, consider catching
(OSError, IOError, ValueError, RuntimeError)as the most common failure modes.
900-901: Consider moving success return toelseblock (optional).Static analysis suggests moving
return image_file.path, display_generationto anelseblock for clarity, distinguishing the success path from exception handling. This is a minor stylistic improvement:♻️ Optional refactor
self.cache_put(cache_key, decoded_image) log.debug(...) - return image_file.path, display_generation except (OSError, IOError, ValueError, MemoryError) as e: log.warning(...) + else: + return image_file.path, display_generation return Nonefaststack/tests/test_metadata.py (2)
9-11: Avoid unused patched arg (mock_exists).
Rename it to_mock_existsto silence ARG002 while keeping the patch.♻️ Proposed fix
- def test_get_exif_data_success(self, mock_open, mock_exists): + def test_get_exif_data_success(self, mock_open, _mock_exists):
68-72: Use bareraisewhen rethrowing.
Keeps the original traceback intact and resolves TRY201.♻️ Proposed fix
- except Exception as e: + except Exception: import traceback traceback.print_exc() - raise e + raisefaststack/tests/test_exif_compat.py (1)
127-138: Drop unusedmock_tiffalias.
The patch is needed, but the bound name isn’t used.♻️ Proposed fix
- patch.object(self.editor, "_write_tiff_16bit") as mock_tiff, + patch.object(self.editor, "_write_tiff_16bit"),faststack/tests/test_exif_orientation.py (2)
24-30: Remove redundant try/except around import.
The exception is immediately re‑raised, so the handler adds no value.♻️ Proposed fix
- try: - from faststack.imaging.editor import ImageEditor - - self.ImageEditorClass = ImageEditor - except ImportError: - # Fallback if path issues persist (shouldn't with sys.path.append) - raise + from faststack.imaging.editor import ImageEditor + self.ImageEditorClass = ImageEditor
179-179: Remove unusedresassignment.
It’s unused and flagged by Ruff F841.♻️ Proposed fix
- res = self.editor.save_image(write_developed_jpg=True) + self.editor.save_image(write_developed_jpg=True)faststack/tests/test_recycle_bin_tracking.py (1)
46-46: Remove unusedtmp_pathparameter.
It’s unused in the test body and triggers ARG001.♻️ Proposed fix
-def test_move_to_recycle_tracks_bin(app_controller, tmp_path): +def test_move_to_recycle_tracks_bin(app_controller):faststack/io/indexer.py (2)
94-110: Unused loop variablestem.The loop iterates over
raws.items()but only usesraw_list. Consider usingraws.values()instead to make the intent clearer.♻️ Suggested fix
- for stem, raw_list in raws.items(): + for raw_list in raws.values(): for raw_path, raw_stat in raw_list: if raw_path not in used_raws:
163-167: Unused parameterjpg_path.The
jpg_pathparameter is not used within the function body. Consider removing it if not needed for future expansion, or prefix with underscore to indicate it's intentionally unused.♻️ Suggested fix
def _find_raw_pair( - jpg_path: Path, + _jpg_path: Path, # Unused but kept for potential future use jpg_stat: os.stat_result, potential_raws: List[Tuple[Path, os.stat_result]], ) -> Path | None:Or remove the parameter entirely and update the call site at line 64.
faststack/thumbnail_view/folder_stats.py (4)
34-39: Duplicate comment on line 38.The comment about
folder_mtime_nsis duplicated on lines 37 and 38.♻️ Suggested fix
# Cache by (folder_path, json_mtime_ns, folder_mtime_ns) to avoid re-parsing during scroll # IMPORTANT: Both json_mtime_ns and folder_mtime_ns are needed: # - json_mtime_ns: changes when faststack.json is modified (flags, metadata) # - folder_mtime_ns: changes when files are added/removed/renamed in folder -# - folder_mtime_ns: changes when files are added/removed/renamed in folder _stats_cache: Dict[Tuple[Path, int, int], Optional[FolderStats]] = {}
98-101: Duplicate comment on line 99.The comment "Cache the result (even if None)" is duplicated.
♻️ Suggested fix
# Cache the result (even if None) - # Cache the result (even if None) _check_cache_size(_stats_cache)
106-120: Naming inconsistency:JPG_EXTENSIONSincludes non-JPG formats.This
JPG_EXTENSIONSset includes PNG, GIF, BMP, TIFF, and WEBP, which is inconsistent with the same-named constant inindexer.pythat only includes actual JPEG extensions. Consider renaming toIMAGE_EXTENSIONSto better reflect its purpose and avoid confusion.
323-324: Cache declaration appears after its first usage.The
_raw_count_cachedictionary is used at line 300 but declared at line 324. Consider moving this declaration near line 39 with the other cache declarations for better code organization.faststack/tests/test_permanent_delete.py (1)
6-12: Use explicitOptionalor| Nonefor type annotation.PEP 484 prohibits implicit
Optional. When a parameter has a default ofNone, the type should explicitly indicate it can beNone.♻️ Suggested fix
+from typing import Optional + + class MockImageFile: """Mock ImageFile for testing.""" - def __init__(self, jpg_path: Path, raw_path: Path = None): + def __init__(self, jpg_path: Path, raw_path: Optional[Path] = None): self.path = jpg_path self.raw_pair = raw_pathOr using Python 3.10+ syntax:
- def __init__(self, jpg_path: Path, raw_path: Path = None): + def __init__(self, jpg_path: Path, raw_path: Path | None = None):faststack/thumbnail_view/model.py (1)
356-364: Avoid swallowing metadata errors silently.
A blindexcept Exceptionhides failures and makes debugging harder. Consider at least a debug log (or narrowing the exception).🔧 Suggested logging tweak
if self._get_metadata: 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) - except Exception: - pass + except Exception as exc: + log.debug("Metadata lookup failed for %s: %s", img.path, exc)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@faststack/io/deletion.py`:
- Around line 33-39: The code assumes image_file.path is non-None when building
files_to_delete (using jpg_path.name) which can raise AttributeError; update the
block that references image_file.path and image_file.raw_pair (jpg_path,
raw_path, files_to_delete) to defend against None by checking if jpg_path is
None before accessing .name — either raise a clear ValueError or skip adding the
JPG to files_to_delete (and log the condition) and then continue to check
raw_path.exists() safely; ensure the chosen behavior (error vs skip) is applied
consistently and documented in the function's docstring or comment.
In `@faststack/ui/provider.py`:
- Around line 139-141: The file defines the signal preloadingStateChanged twice
which causes the first definition to be shadowed; remove the duplicate
declaration so there is only a single preloadingStateChanged = Signal() (keep
the other signals like preloadProgressChanged intact) and ensure any references
in the class/module use that single symbol.
🧹 Nitpick comments (4)
faststack/tests/check_imports.py (1)
23-37: LGTM - same approved pattern as above.The second import check follows the same structure. One optional cleanup: consider moving
import tracebackto the top of the file alongside the other imports to avoid the repeated conditional imports.,
♻️ Optional: Move traceback import to top
import sys import os +import traceback # Add current directory to path sys.path.append(os.getcwd())Then remove the
import tracebacklines from each except block (lines 14, 19, 30, 35).faststack/io/watcher.py (1)
47-51: Unusedself.callbackattribute.The
callbackis stored inself.callbackon line 51, but it's never used elsewhere in the class. The callback is already passed toImageDirectoryEventHandleron line 49. Consider removing the unused assignment to avoid confusion.♻️ Proposed fix
def __init__(self, directory: Path, callback): self.observer: Optional[Observer] = None # Initialize to None self.event_handler = ImageDirectoryEventHandler(callback) self.directory = directory - self.callback = callback # Store callback for new observerfaststack/io/deletion.py (1)
19-21: Uselogging.exceptionto preserve stack trace.When catching exceptions,
log.exceptionautomatically includes the traceback, which is valuable for debugging. This applies here and in thepermanently_delete_image_filesfunction (lines 142, 151).♻️ Proposed fix
except (PermissionError, OSError) as e: - log.error("Failed to create recycle bin directory: %s", e) + log.exception("Failed to create recycle bin directory: %s", e) return FalseAnd similarly in
permanently_delete_image_files:except OSError as e: - log.error("Failed to permanently delete %s: %s", jpg_path.name, e) + log.exception("Failed to permanently delete %s: %s", jpg_path.name, e)faststack/tests/test_permanent_delete.py (1)
14-19: Use explicitPath | Nonetype hint instead of implicitOptional.PEP 484 prohibits implicit
Optional. Theraw_pathparameter should use an explicit union type.♻️ Proposed fix
class MockImageFile: """Simple mock for ImageFile.""" - def __init__(self, jpg_path: Path, raw_path: Path = None): + def __init__(self, jpg_path: Path, raw_path: Path | None = None): self.path = jpg_path self.raw_pair = raw_path self.is_video = False
| jpg_path = image_file.path | ||
| raw_path = image_file.raw_pair | ||
|
|
||
| # Build list of files that will be deleted | ||
| files_to_delete = [str(jpg_path.name)] | ||
| if raw_path and raw_path.exists(): | ||
| files_to_delete.append(str(raw_path.name)) |
There was a problem hiding this comment.
Potential AttributeError if image_file.path is None.
Line 37 accesses jpg_path.name without checking if jpg_path is None. If image_file.path could be None, this will raise an AttributeError. Consider adding a guard or documenting that image_file.path must be non-None.
🛡️ Proposed defensive fix
jpg_path = image_file.path
raw_path = image_file.raw_pair
# Build list of files that will be deleted
- files_to_delete = [str(jpg_path.name)]
+ files_to_delete = [str(jpg_path.name)] if jpg_path else []
if raw_path and raw_path.exists():
files_to_delete.append(str(raw_path.name))
+
+ if not files_to_delete:
+ return False # Nothing to delete🤖 Prompt for AI Agents
In `@faststack/io/deletion.py` around lines 33 - 39, The code assumes
image_file.path is non-None when building files_to_delete (using jpg_path.name)
which can raise AttributeError; update the block that references image_file.path
and image_file.raw_pair (jpg_path, raw_path, files_to_delete) to defend against
None by checking if jpg_path is None before accessing .name — either raise a
clear ValueError or skip adding the JPG to files_to_delete (and log the
condition) and then continue to check raw_path.exists() safely; ensure the
chosen behavior (error vs skip) is applied consistently and documented in the
function's docstring or comment.
| preloadingStateChanged = Signal() | ||
| preloadingStateChanged = Signal() | ||
| preloadProgressChanged = Signal() |
There was a problem hiding this comment.
Duplicate signal declaration will shadow the first definition.
preloadingStateChanged is declared twice (lines 139 and 140). The second declaration shadows the first. Remove the duplicate.
🐛 Proposed fix
preloadingStateChanged = Signal()
- preloadingStateChanged = Signal()
preloadProgressChanged = Signal()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| preloadingStateChanged = Signal() | |
| preloadingStateChanged = Signal() | |
| preloadProgressChanged = Signal() | |
| preloadingStateChanged = Signal() | |
| preloadProgressChanged = Signal() |
🤖 Prompt for AI Agents
In `@faststack/ui/provider.py` around lines 139 - 141, The file defines the signal
preloadingStateChanged twice which causes the first definition to be shadowed;
remove the duplicate declaration so there is only a single
preloadingStateChanged = Signal() (keep the other signals like
preloadProgressChanged intact) and ensure any references in the class/module use
that single symbol.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.