Release versi#43
Conversation
…at caused Helicon to not load the files sometimes
WalkthroughThis PR introduces a comprehensive variant management system for image files (developed/backup variants), adds full-screen loupe mode with F11 support and spark line progress visualization, enhances thumbnail prefetching with detailed timing/trace debugging, improves cache eviction to prevent lock contention, and includes extensive test coverage for new functionality. Version bumped to 1.5.9. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as QML (Main.qml)
participant AppCtrl as AppController
participant Prefetch as Prefetcher
participant Editor as ImageEditor
User->>UI: Click variant badge or setVariantOverride()
UI->>AppCtrl: set_variant_override(path_str)
activate AppCtrl
AppCtrl->>AppCtrl: _variant_overrides[current_index] = path
AppCtrl->>AppCtrl: get_variant_badges() for UI update
AppCtrl-->>UI: variantBadgesChanged signal
deactivate AppCtrl
User->>UI: Trigger save (Ctrl+S)
UI->>AppCtrl: save_image() via signal
activate AppCtrl
AppCtrl->>AppCtrl: _get_save_target_path_for_current_view()
note over AppCtrl: Returns main, developed, or backup path<br/>based on variant override
AppCtrl->>Editor: save_image(save_target_path=computed_path)
activate Editor
Editor->>Editor: resolve original_path from save_target_path
Editor->>Editor: compute rotation/straighten from edits
Editor->>Editor: write JPEG/PNG to save_target_path
Editor-->>AppCtrl: returns (path, developed_path) or error
deactivate Editor
AppCtrl->>Prefetch: decode override_path for display
activate Prefetch
Prefetch->>Prefetch: submit_task(override_path=computed_path)
Prefetch-->>AppCtrl: future with decoded image
deactivate Prefetch
AppCtrl-->>UI: image_changed signal with new decoded data
deactivate AppCtrl
UI->>UI: Update loupe display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
faststack/imaging/prefetch.py (1)
340-397:⚠️ Potential issue | 🔴 CriticalTOCTOU: bounds check on stale snapshot, live access inside lock.
Lines 341-343 snapshot
self.image_filesand check bounds outside the lock, but line 383 accessesself.image_files[index](the live reference) inside the lock. Ifset_image_filesreplacesself.image_fileswith a shorter list between lines 343 and 383, this will raiseIndexError.Move the snapshot and bounds check inside the lock, or re-check bounds on the live list inside the lock before accessing line 383.
Proposed fix
def submit_task( self, index: int, generation: int, priority: bool = False, override_path: Optional[Path] = None ) -> Optional[Future]: """Submits a decoding task for a given index.""" if self._stop_event.is_set(): return None - # Capture list snapshot and check bounds before accessing path - image_files = self.image_files - if index < 0 or index >= len(image_files): - return None - - requested_path = override_path if override_path is not None else image_files[index].path - if self.debug and priority: _t_start = time.perf_counter() print(f"[DBGCACHE] {_t_start*1000:.3f} submit_task: PRIORITY index={index} gen={generation} override={override_path}") with self._futures_lock: + # Bounds check must happen inside the lock to stay consistent + # with self.image_files (which set_image_files can replace under lock). + if index < 0 or index >= len(self.image_files): + return None + + requested_path = override_path if override_path is not None else self.image_files[index].path + # We track by index. If we already have a job for this index, # we must cancel it if the requested path is different # (e.g. switching between main and variants).faststack/app.py (1)
805-845:⚠️ Potential issue | 🟠 MajorFix load() signature mismatch with directory switching.
_switch_to_directory()still callsload(skip_thumbnail_refresh=True)butload()no longer accepts that parameter, which will raise aTypeErrorat runtime. Please reintroduce the parameter or remove the call‑site argument.🧩 Proposed fix (restore param + guard refresh)
- def load(self): - """Loads images, sidecar data, and starts services.""" + def load(self, skip_thumbnail_refresh: bool = False): + """Loads images, sidecar data, and starts services.""" # Reset instrumentation for this load operation self._scan_count_variant = 0 self._grid_refreshes = 0 self._grid_model_dirty = True @@ - if self._is_grid_view_active: + if self._is_grid_view_active and not skip_thumbnail_refresh: # Ensure grid model is populated if starting in grid mode if self._thumbnail_model and self._grid_model_dirty and self._thumbnail_model.rowCount() == 0: self._grid_refreshes += 1 self._thumbnail_model.refresh_from_controller(self.image_files)
🤖 Fix all issues with AI agents
In `@faststack/app.py`:
- Around line 4738-4763: The _on_cache_evict callback currently declares
parameters key and value but doesn't use them, triggering a lint error; fix by
either renaming the parameters to _key and _value in the _on_cache_evict
signature to mark them intentionally unused, or incorporate them into the
warning/log and UI update (e.g., include key or a short value summary in the msg
passed to self.update_status_message and log.warning); adjust references inside
_on_cache_evict (and the lambda to self.update_status_message) accordingly so
the chosen approach is consistent.
- Around line 6908-6911: The global _debug_thumb_trace is declared but never
updated, so when the CLI flag --debug-thumbtrace is used the module-level
_debug_thumb_trace remains False; update the same block that sets _debug_mode
and _debug_thumb_timing to also set _debug_thumb_trace = debug_thumb_trace
(declared as global at top of that block) so the flag value is propagated to the
module state; ensure you reference the existing globals _debug_mode,
_debug_thumb_timing, and _debug_thumb_trace and use the incoming parameter name
debug_thumb_trace.
In `@faststack/check_daemon.py`:
- Around line 14-15: The module currently creates a ThreadPoolExecutor and
submits check_daemon at import time (using ThreadPoolExecutor, set_daemon, and
check_daemon); move that runtime behavior into a script entry point by wrapping
the executor block in an if __name__ == "__main__": guard so the
ThreadPoolExecutor(max_workers=1, initializer=set_daemon) creation and
executor.submit(check_daemon).result() call only run when the file is executed
as a script, preserving the same print behavior but preventing side effects on
import.
In `@faststack/debug_path_norm.py`:
- Around line 1-22: This debug script contains a duplicate norm_path function
and module-level print statements that run on import; remove this file
(faststack/debug_path_norm.py) from the PR or at minimum delete the debugging
code: eliminate the duplicate norm_path definition and all top-level print/test
code so the module has no side effects on import; if you want to keep the
behavior, move the example into a proper unit test or a __main__ guarded block
(if truly needed) and ensure only the canonical norm_path implementation remains
in its original module (norm_path symbol in faststack/io/variants.py).
In `@faststack/imaging/cache.py`:
- Around line 13-33: The fallback in get_decoded_image_size incorrectly assumes
4 channels because DecodedImage has no channels attribute; update the fallback
to compute bytes_per_pixel from available image metadata: if item has
bytes_per_line (or a similar attribute) and width, derive bytes_per_pixel =
bytes_per_line / width and use that (rounded or cast to int) as
channels/bytes-per-pixel, otherwise fall back to 4; adjust the branch inside
get_decoded_image_size that currently uses getattr(item, "channels", 4) to first
check for item.bytes_per_line and item.width and compute channels from that
before defaulting.
In `@faststack/io/indexer.py`:
- Around line 125-137: The timing variable elapsed in find_images is computed
but never logged; update the log.info call in function find_images to include
elapsed (e.g., append "in %.2fs" or similar) so the elapsed value is printed
alongside the image counts—refer to the existing log.info invocation that
currently logs len(image_files), paired_count, raw_only_count and add elapsed to
the message and its arguments to mirror the behavior in
find_images_with_variants.
- Around line 168-187: The comparison between img.path and group.main_path can
fail because group.main_path is normalized (Path(norm_path(...))) in
build_variant_map (variants.py) while img.path comes from _scan_directory and
may be relative; update the filter logic in indexer.py to compare normalized
paths: compute a normalized Path for img.path using the same norm_path helper
used in build_variant_map (or otherwise resolve/absolutize and casefold
consistently), then use that normalized value when checking equality against
group.main_path (refer to img.path, group.main_path, variant_map,
parse_variant_stem and JPG_EXTENSIONS to locate the code). Ensure you do not
mutate the original img.path unless intended.
In `@faststack/tests/test_handle_failures_isolated.py`:
- Around line 30-32: The two `@patch` decorators for confirm_permanent_delete and
confirm_batch_permanent_delete are applied bottom-up so the innermost
(confirm_batch_permanent_delete) is passed as the first argument to
test_handle_delete_failures_recycle_codes_isolation; rename the test parameters
to match that order (e.g., first param mock_confirm_batch_permanent_delete,
second param mock_confirm_permanent_delete) or swap their positions so the
parameter names align with the patched functions.
In `@faststack/tests/test_startup_opt.py`:
- Around line 83-85: Remove the two debug print statements around the
apply_filter call: delete the lines printing f"DEBUG: before apply_filter:
grid_active={controller._is_grid_view_active},
dirty={controller._grid_model_dirty}" and the matching "after" print; if you
need diagnostic output keep it via the test framework or use logging (e.g.,
replace prints with logger.debug) but do not leave raw print calls next to
controller.apply_filter("test", []).
- Around line 34-35: The test is asserting controller._scan_count_simple which
does not exist on AppController; remove the assertion line referencing
_scan_count_simple from faststack/tests/test_startup_opt.py so the test only
asserts the existing controller._scan_count_variant (and any other valid
attributes), or alternatively add initialization of _scan_count_simple in
AppController.__init__ if that counter is actually required; reference symbols:
controller, _scan_count_simple, _scan_count_variant, AppController.__init__.
In `@faststack/tests/test_ui_prefetch_safety.py`:
- Line 29: The test assigns self.model.thumbnail_size = (256, 256) but
ThumbnailModel.thumbnail_size is an int; change the assignment in the test to an
integer (e.g., 256) so the test matches the real property type and surfaces any
type-dependent logic (refer to ThumbnailModel.thumbnail_size and to the test's
use of prefetcher.submit which is mocked).
In `@faststack/tests/test_variants_logic.py`:
- Line 10: The comment "Cas 1: Pure Main exists" contains a typo; update that
comment in faststack/tests/test_variants_logic.py to read "Case 1: Pure Main
exists" (replace "Cas 1" with "Case 1") so the test comment is spelled
correctly.
In `@faststack/thumbnail_view/model.py`:
- Around line 147-156: Remove the duplicate initialization of the instance
variable _next_source_reason: locate the duplicate assignments to
_next_source_reason (both set to Optional[str] = None) in the class initializer
and delete the redundant second one so the variable is only initialized once;
ensure _id_to_row and surrounding comments remain unchanged and run tests/lint
to confirm no other references relied on the duplicate.
In `@faststack/ui/provider.py`:
- Around line 559-562: The variantSaveHint property calls
self.app_controller.get_variant_save_hint() unguarded and can raise
AttributeError; update the variantSaveHint getter to check
hasattr(self.app_controller, "get_variant_save_hint") and return a safe default
(e.g., "" or None) when missing, mirroring the pattern used by variantBadges;
ensure you keep the `@Property` decorator and notify=variantSaveHintChanged and
only call get_variant_save_hint() when the attribute exists.
In `@README.md`:
- Line 15: Replace the incorrect phrase "Speak Line" with "Spark Line" wherever
it appears (starting with the README entry that currently reads "**Speak Line**:
In grid view...") and also update any references in the PR description or AI
summary to use "Spark Line" so the terminology is consistent across docs.
🧹 Nitpick comments (37)
faststack/qml/Components.qml (1)
34-38: Guard againstrootbeing undefined.If
loupeViewis ever instantiated outside the context whererootis defined (e.g., during testing or if the component hierarchy changes), accessingroot.fullScreenLoupewill throw a ReferenceError. Consider adding a null guard similar to theuiState &&pattern used elsewhere.- if (root.fullScreenLoupe) { - root.exitFullScreenLoupe() + if (typeof root !== "undefined" && root && root.fullScreenLoupe) { + root.exitFullScreenLoupe()Note: This is consistent with the defensive pattern already used on line 1266 (
typeof root !== "undefined" && root)..gitignore (1)
85-90: Overlapping glob patterns for test output files.Line 85 (
test_output*.txt) covers suffixed variants while line 90 (*test_output.txt) covers prefixed variants. These overlap on the exact filenametest_output.txt. Not a problem functionally, but worth noting the redundancy.README.md (1)
30-30: Minor: "full screen" as a compound adjective should be hyphenated.Per static analysis hint and standard English grammar, "full-screen mode" is the correct compound adjective form.
-- **Full Screen Mode:** Pressing F11 enters full screen mode - Esc/F11 exits. +- **Full-Screen Mode:** Pressing F11 enters full-screen mode — Esc/F11 exits.faststack/repro_cache_lock.py (1)
1-47: Debug/repro script looks reasonable.The approach of using a separate thread to verify lock state during eviction is sound. Two minor observations:
- The
on_evict_callbackparameterskeyandvalueare required by the callback contract but unused — consider prefixing with_to silence the Ruff ARG001 warnings.- There's a potential race: the
check_lockthread could run after the eviction callback's caller has already released the lock, producing a false negative. Thet.join()mitigates this, but if eviction is very fast, timing could still vary.Silence unused arg warnings
- def on_evict_callback(key, value): + def on_evict_callback(_key, _value):faststack/util/executors.py (1)
60-72: Return type annotation could be more specific.The factory function returns
DaemonThreadPoolExecutorbut the return type is annotated asThreadPoolExecutor. This is fine for polymorphism but could be tightened to match the docstring on Line 65.Proposed fix
def create_daemon_threadpool_executor( max_workers: int, thread_name_prefix: str = "" -) -> ThreadPoolExecutor: +) -> DaemonThreadPoolExecutor:faststack/imaging/cache.py (1)
217-276:evict_pathsdoes not invokeon_evictcallbacks for removed entries.
super().pop(k, None)at Line 265 bypasses the overriddenpopitem()and its callback logic. If consumers rely onon_evictfor cleanup (e.g., releasing GPU textures), evicted entries fromevict_pathswill be missed. If this is intentional (tombstoned paths don't need eviction signaling), a brief comment would clarify.faststack/verify_cache_fix.py (1)
1-10: Imports use bare module names without thefaststackpackage prefix.Lines 8–9 import from
imaging.cacheandmodelsinstead offaststack.imaging.cacheandfaststack.models. This relies on the fragilesys.path.append(os.getcwd())on Line 6 and will fail if run from a different working directory.Proposed fix
-# Add current dir to path -sys.path.append(os.getcwd()) - -from imaging.cache import ByteLRUCache -from models import DecodedImage +from faststack.imaging.cache import ByteLRUCache +from faststack.models import DecodedImagefaststack/imaging/editor.py (3)
1954-1957: Redundant exception handler —RuntimeErroris immediately re-raised.The
try/except RuntimeError: raiseblock is a no-op; removing it lets the exception propagate naturally.Proposed fix
- try: - self._ensure_float_image() - except RuntimeError: - raise + self._ensure_float_image()
2131-2141:save_image_uint8_levelsreadsself.current_editswithout lock, unlikesave_image.Line 2141 accesses
self.current_editsdirectly without holdingself._lock, whilesave_image(Line 1984) snapshots edits under the lock. Since this fast-path only reads primitive float values and any mid-read race would safely fall through toreturn None, this is low-risk but inconsistent.
1610-1656: Newsrgb_u8alias parameter is unused at the call site.The
srgb_u8keyword-only alias is declared (Line 1617) and resolved intoeffective_srgb_u8(Line 1644), but the only call site in_apply_edits(Lines 1030–1037) still passessrgb_u8_stride=srgb_u8_strideonly. If this is intentional prep for a future caller, consider adding a brief comment or TODO.faststack/imaging/prefetch.py (1)
456-464: Silentexcept Exception: passswallows all errors during EXIF read.Static analysis flags this at lines 461-462. While silently falling back to
orientation=1may be intentional for robustness, logging atdebuglevel would aid troubleshooting without adding noise.Proposed fix
except Exception: - pass + log.debug("Failed to read EXIF from mmap for %s", target_path, exc_info=True)Same applies to the analogous block at lines 531-532.
faststack/tests/test_thumbnail_ready_emits_datachanged.py (1)
12-18: Session-scopedqappfixture with[]instead ofsys.argv.The sibling fixture in
test_thumbnail_ready.pyusesQCoreApplication(sys.argv), while this one usesQCoreApplication([]). Both work, but usingsys.argvis more conventional for Qt apps that may need to parse args. Minor inconsistency.faststack/qml/ThumbnailTile.qml (2)
253-272: Variant badges look correct; consider consistent formatting with existing badges.The new variant badge rows work correctly. However, the inline multi-property style (
width: 18; height: 18; radius: 3; ...) differs from the multi-line format used by the existing flag badges (lines 157-250). Consider aligning the formatting for consistency, though this is purely stylistic.
502-505: Debug logging gated properly, butuiStatereference is not null-safe in all contexts.If
ThumbnailTileis ever instantiated in a context whereuiStateis not injected (e.g., a test harness or different QML loader), theuiState && uiState.debugThumbTimingcheck guards against undefined, but QML may still emit a warning for an unresolved reference. This is minor since it only fires for index 0 with debug enabled.faststack/tests/test_handle_failures_isolated.py (2)
25-28: Class-body import ofAppControlleris fragile.Importing
AppControllerat class-body scope (line 27) triggers the full import offaststack.appat module load time during class definition. Iffaststack.apphas heavy side effects or circular imports, this will break. Consider deferring the binding tosetUpor a fixture.
8-9: Module-levelMagicMockdeclarations appear to be dead code.
confirm_permanent_deleteandconfirm_batch_permanent_deleteat lines 8-9 are never referenced—the@patchdecorators on the test function replace the real functions infaststack.appdirectly. These can be removed.Proposed fix
-# Mocks for global functions that might be called -confirm_permanent_delete = MagicMock(return_value=True) -confirm_batch_permanent_delete = MagicMock(return_value=True) -faststack/tests/test_helicon_cleanup.py (1)
9-33: Patches are not active when the fixture's return value is used by the test.The
with patch(...)context manager exits at the end of thewithblock (line 32), butreturn controlleris on line 33, outside the block. This means the module-level patches are restored before the test body runs. It works here because__init__already bound mock objects to the controller's instance attributes, and the test separately patcheslaunch_helicon_focus. However, this is fragile — if any code path in the test triggers a fresh lookup on thefaststack.appmodule (e.g., a lazy import or factory call), it will get the real class instead of the mock.Consider yielding the controller from inside the
withblock to keep patches active during the test:Suggested fix
`@pytest.fixture` def mock_controller(): # Mock dependencies required by AppController init engine = MagicMock() with patch("faststack.app.Watcher"), \ patch("faststack.app.SidecarManager"), \ patch("faststack.app.ImageEditor"), \ patch("faststack.app.ByteLRUCache"), \ patch("faststack.app.Prefetcher"), \ patch("faststack.app.ThumbnailCache"), \ patch("faststack.app.PathResolver"), \ patch("faststack.app.ThumbnailPrefetcher"), \ patch("faststack.app.ThumbnailModel"), \ patch("faststack.app.ThumbnailProvider"), \ patch("faststack.app.concurrent.futures.ThreadPoolExecutor"), \ patch("faststack.app.QTimer"), \ patch("faststack.app.QApplication"): # Mock QApplication to avoid segfaults controller = AppController(image_dir=Path("c:/images"), engine=engine) # Setup dummy images for the controller img1 = ImageFile(Path("c:/images/img1.jpg")) controller.image_files = [img1] - return controller + yield controllerfaststack/tests/test_variants.py (1)
74-85: Address static analysis warnings for unused unpacked variables.Multiple test methods unpack variables that are never used (e.g.,
vmap,backup,dev,images). Prefixing with_silences the linter and communicates intent. Also, list-comprehension-then-[0]on lines 302 and 314 can be replaced withnext(...)for clarity.Examples
# Lines 74-78 — unused `backup` - key, dev, backup = parse_variant_stem("photo-undeveloped") + key, dev, _backup = parse_variant_stem("photo-undeveloped") # Lines 93-96 — unused `dev` - key, dev, backup = parse_variant_stem("photo-BACKUP3") + key, _dev, backup = parse_variant_stem("photo-BACKUP3") # Lines 262, 274, 286, 301, 313 — unused `vmap` / `images` - images, vmap = find_images_with_variants(tmp_path) + images, _vmap = find_images_with_variants(tmp_path) # (or `_images, vmap = ...` where images is unused) # Lines 302, 314 — prefer next() - photo = [img for img in images if img.path.name == "photo.jpg"][0] + photo = next(img for img in images if img.path.name == "photo.jpg")Also applies to: 93-96, 262-266, 274-276, 286-291, 301-305, 313-317, 326-329
faststack/tests/test_startup_opt.py (1)
29-41: Potential duplication withtest_startup_optimization.py.
test_startup_only_one_scantests the same load-time invariants (variant scan count, grid refreshes,refresh_from_controllercalled once) astest_startup_optimizationin the other file. Consider consolidating to avoid divergent expectations.faststack/tests/test_startup_optimization.py (2)
12-16: Unused variableappand unused importos.Line 15:
app = QApplication([])— the variable is never read; the instance is available viaQApplication.instance(). Line 2:osis imported but never used.Suggested fix
-import sys -import os +import sys from pathlib import Path`@pytest.fixture`(scope="session", autouse=True) def qapplication(): if not QApplication.instance(): - app = QApplication(sys.argv) + QApplication(sys.argv) yield QApplication.instance()
18-66: Duplicate coverage withtest_startup_opt.py::test_startup_only_one_scan.This test validates the same invariants (variant scan count == 1, grid refreshes == 1,
refresh_from_controllercalled once,refreshnot called) as the test intest_startup_opt.py. They use different mocking strategies (decorator-based patches here vs. fixture with context-manager patches there), which can lead to maintenance burden and divergent expectations over time. Consider keeping one canonical test file for startup optimization.faststack/thumbnail_view/provider.py (2)
144-251: Significant code duplication between timer and non-timer paths inrequestPixmap.The ID parsing logic (split parts, length check, folder check, parse
thumb_size/path_hash/mtime_ns) appears twice: once inside the timer branch (lines 163-178) and again in the non-timer fallback (lines 222-242). Additionally,pathis resolved at line 182 (timer path) and then unconditionally resolved again at line 245, overwriting the previous result.Consider extracting the common parsing into a small helper to reduce duplication and the risk of the two paths drifting out of sync.
190-192: Unconditionaltime.perf_counter()calls on the hot path.
t_cache_get_startanddt_cache_getare computed on every request, but they're only consumed whentimeris set (lines 195-197, 214-216). On the non-debug fast path, this is wasted work. Whileperf_counteris lightweight, this method is called for every visible thumbnail.Suggested fix
# Check cache (O(1) lookup) - t_cache_get_start = time.perf_counter() + t_cache_get_start = time.perf_counter() if timer else 0 cached_bytes = self._cache.get(cache_key) - dt_cache_get = (time.perf_counter() - t_cache_get_start) * 1000 + dt_cache_get = (time.perf_counter() - t_cache_get_start) * 1000 if timer else 0faststack/tests/test_feedback_fixes.py (2)
47-52: Remove unusedtmp_pathparameter.
tmp_pathis injected by pytest but never used. This was flagged by Ruff (ARG001).Suggested fix
-def test_image_editor_save_exception(tmp_path): +def test_image_editor_save_exception():
73-93: Test files created withwrite_text("dummy")instead of valid JPEG bytes.Other tests in this PR (e.g.,
test_variants.py) usewrite_bytes(b"\xff\xd8\xff\xe0")to create minimal JPEG files. Here,write_text("dummy")produces non-JPEG content. Iffind_images_with_variantsever adds JPEG header validation, this test will silently break. Consider using consistent file creation.Suggested fix
- orphan = tdp / "orphan-developed.jpg" - orphan.write_text("dummy") + orphan = tdp / "orphan-developed.jpg" + orphan.write_bytes(b"\xff\xd8\xff\xe0") # Also create a regular pair for comparison pair_main = tdp / "pair.jpg" pair_dev = tdp / "pair-developed.jpg" - pair_main.write_text("dummy") - pair_dev.write_text("dummy") + pair_main.write_bytes(b"\xff\xd8\xff\xe0") + pair_dev.write_bytes(b"\xff\xd8\xff\xe0")faststack/util/thumb_debug.py (1)
192-202: Useis not Noneinstead of truthiness checks forperf_countertimestamps.
self.t_queued,self.t_worker_start, andself.t_doneareOptional[float]timestamps fromtime.perf_counter(). Checkingif self.t_queued:would incorrectly skip the block if the value happened to be0.0. While extremely unlikely withperf_counter, usingis not Noneis the correct and idiomatic check for optionals.Suggested fix
- if self.t_queued: + if self.t_queued is not None: sched_ms = (self.t_queued - self.t_requested) * 1000 parts.append(f"sched_ms={sched_ms:.1f}") - if self.t_worker_start: + if self.t_worker_start is not None: wait_ms = (self.t_worker_start - self.t_queued) * 1000 parts.append(f"wait_ms={wait_ms:.1f}") - if self.t_done: + if self.t_done is not None: worker_ms = (self.t_done - self.t_worker_start) * 1000 parts.append(f"worker_ms={worker_ms:.1f}")faststack/qml/Main.qml (1)
815-827: Minor redundancy: Escape is handled by both the Shortcut andKeys.onPressed.The
Shortcutat line 822 (withQt.ApplicationShortcutcontext) handles Escape whenfullScreenLoupeis true. TheKeys.onPressedhandler at lines 916–920 also checksfullScreenLoupeand handles Escape. In practice the Shortcut fires first and setsfullScreenLoupe = false, so the Keys handler becomes a no-op. Not a bug, just redundant defense-in-depth.faststack/io/variants.py (3)
57-64: Redundant regex search — reuse the first match object.
_DEVELOPED_TOKEN_RE.search(stem)is called twice: once on line 58 to check presence, and again on line 63 to get the match. Save the first result and reuse it.♻️ Proposed fix
- # 1. Check for -developed token - is_developed = bool(_DEVELOPED_TOKEN_RE.search(stem)) - - # 2. Remove exactly one -developed token (first occurrence) -> stripped - if is_developed: - # Find the match and remove it, handling leading/trailing dashes - m = _DEVELOPED_TOKEN_RE.search(stem) + # 1. Check for -developed token + m = _DEVELOPED_TOKEN_RE.search(stem) + is_developed = m is not None + + # 2. Remove exactly one -developed token (first occurrence) -> stripped + if is_developed:
236-238: Duplicatenorm_path— identical implementation exists infaststack/debug_path_norm.py.The relevant snippet shows
faststack/debug_path_norm.pydefines the samenorm_path(p: Path) -> strwith the same body. Consider extracting into a shared utility to avoid drift.#!/bin/bash # Check all definitions of norm_path across the codebase rg -n "def norm_path" --type=py -C2
104-117: Paths are normalized at insertion butgroup_keyuses the first-seen casing.Line 116 sets
group_key=group_keyfrom whichever file is encountered first. If files have different casings of the same stem (e.g.,Foo-developed.jpgvsfoo.jpg), the group's displaygroup_keydepends on iteration order (which isos.scandirorder, i.e., filesystem-dependent). This is cosmetic only since lookups usecasefold(), but worth noting.faststack/thumbnail_view/prefetcher.py (4)
262-265: PossibleAttributeErroriftimerisNone.The
_decode_workertype hint saystimer: "thumb_debug.ThumbTimer"(non-Optional), but line 263 accessestimer.cancelledwithout a guard, while other call sites (e.g., line 389) defensively checktimer and timer.cancelled. In practice,submit()always passes a non-None timer, so this is safe today. Just ensure the invariant holds if the method is ever called differently.
278-286: Moveimport ioto the top of the file.
import ioon line 282 is inside the hot decode path. While Python caches imports (making repeated imports cheap), placing it at the module level is the standard convention and avoids the per-callsys.moduleslookup.♻️ Proposed fix
Add at the top of the file alongside other stdlib imports:
import ioThen remove line 282.
309-340:timer.stage("decode")is entered twice for the TurboJPEG path.When TurboJPEG succeeds but further PIL resizing is needed (line 335), a second
timer.stage("decode")context is entered. Depending on howThumbTimer.stageaccumulates or overwrites timings, this may produce misleading metrics (overwriting the TurboJPEG decode duration or double-counting). Consider using a distinct stage name like"resize"for the PIL thumbnail step.♻️ Proposed fix
if w > target_size or h > target_size: - with timer.stage("decode"): + with timer.stage("resize"): pil_img = Image.fromarray(rgb)
192-193:"inflight"and"qdepth"gauges are always set to the same value.Both gauges at lines 192–193 (and again at 372–373, 426–427) track
len(self._inflight). If they're meant to represent different concepts (e.g., in-flight = actively executing, qdepth = waiting in queue), they should diverge. Otherwise, one of them is redundant and adds noise to your metrics.faststack/io/indexer.py (1)
213-227: Conditional logging:log.infoinsidelog.isEnabledFor(logging.DEBUG)block.The detailed elapsed-time log (line 214) is gated behind
logging.DEBUG, but it logs atINFOlevel. This means the elapsed time is only visible when the logger is set to DEBUG, even though the message severity is INFO. If the intent is to always log elapsed time at INFO level, remove theisEnabledForguard. If the intent is verbose-only output, consider logging at DEBUG level instead.faststack/qml/ThumbnailGridView.qml (1)
157-166:onCountChangedrestartsprefetchTimerregardless ofprefetchEnabled.When
prefetchEnabledis false,triggerPrefetch()bails early (line 116), so this is harmless. But it fires a 50ms timer for no reason. Consider guarding this restart:♻️ Proposed fix
onCountChanged: { if (count <= 0) { currentIndex = 0 return } if (currentIndex >= count) { currentIndex = count - 1 } - prefetchTimer.restart() + if (prefetchEnabled) prefetchTimer.restart() }faststack/thumbnail_view/model.py (1)
206-209: One-shot reason is consumed on the firstThumbnailSourceRoleaccess per batch.When QML iterates the model after a refresh/filter, only the first entry's thumbnail URL will carry the actual
reason(e.g.,"filter","jump"); all subsequent entries in the same pass will fall back to"scroll". If this is intentional for lightweight debugging, it's fine. If per-entry reason tracking matters, the reset should happen at the call site (e.g., afterendResetModel).
| def _on_cache_evict(self, key, value): | ||
| """Callback for when the image cache evicts an item.""" | ||
| now = time.time() | ||
|
|
||
| # 1. Record eviction timestamp | ||
| self._eviction_timestamps.append(now) | ||
|
|
||
| # 2. Prune timestamps older than window | ||
| # Keep list short | ||
| cutoff = now - CACHE_THRASH_WINDOW_SECS | ||
| self._eviction_timestamps = [t for t in self._eviction_timestamps if t > cutoff] | ||
|
|
||
| # 3. Check for thrashing (e.g., > threshold evictions in window) | ||
| if len(self._eviction_timestamps) > CACHE_THRASH_THRESHOLD: | ||
| # 4. Rate limit the warning | ||
| if now - self._last_cache_warning_time > CACHE_WARNING_COOLDOWN_SECS: | ||
| self._last_cache_warning_time = now | ||
| self._has_warned_cache_full = True | ||
|
|
||
| # Format usage info | ||
| used_gb = self.image_cache.currsize / (1024**3) | ||
| max_gb = self.image_cache.max_bytes / (1024**3) | ||
|
|
||
| msg = f"Cache thrashing! {len(self._eviction_timestamps)} evictions in {CACHE_THRASH_WINDOW_SECS}s. Usage: {used_gb:.1f}GB / {max_gb:.1f}GB." | ||
|
|
||
| # Use QTimer.singleShot to ensure this runs on the main thread | ||
| QTimer.singleShot(0, lambda: self.update_status_message(msg)) | ||
| log.warning(msg) | ||
| with self._eviction_lock: | ||
| # 1. Record eviction timestamp / prune | ||
| self._eviction_timestamps.append(now) | ||
| cutoff = now - CACHE_THRASH_WINDOW_SECS | ||
| self._eviction_timestamps = [t for t in self._eviction_timestamps if t > cutoff] | ||
|
|
||
| # 2. Check for thrashing (e.g., > threshold evictions in window) | ||
| if len(self._eviction_timestamps) > CACHE_THRASH_THRESHOLD: | ||
| # 3. Rate limit the warning | ||
| if now - self._last_cache_warning_time > CACHE_WARNING_COOLDOWN_SECS: | ||
| self._last_cache_warning_time = now | ||
| self._has_warned_cache_full = True | ||
|
|
||
| # UI update logic | ||
| used_gb = self.image_cache.currsize / (1024**3) | ||
| max_gb = self.image_cache.max_bytes / (1024**3) | ||
| msg = f"Cache thrashing! {len(self._eviction_timestamps)} evictions in {CACHE_THRASH_WINDOW_SECS}s. Usage: {used_gb:.1f}GB / {max_gb:.1f}GB." | ||
|
|
||
| # Schedule UI work safely on main thread | ||
| # QTimer.singleShot(0, ...) is thread-safe entry to main loop | ||
| QTimer.singleShot(0, lambda: self.update_status_message(msg)) | ||
| log.warning(msg) |
There was a problem hiding this comment.
Lint: unused key/value in cache eviction callback.
Ruff flags these as unused; please either use them (e.g., in log context) or rename to _key, _value.
✅ Simple lint fix
- def _on_cache_evict(self, key, value):
+ def _on_cache_evict(self, _key, _value):📝 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.
| def _on_cache_evict(self, key, value): | |
| """Callback for when the image cache evicts an item.""" | |
| now = time.time() | |
| # 1. Record eviction timestamp | |
| self._eviction_timestamps.append(now) | |
| # 2. Prune timestamps older than window | |
| # Keep list short | |
| cutoff = now - CACHE_THRASH_WINDOW_SECS | |
| self._eviction_timestamps = [t for t in self._eviction_timestamps if t > cutoff] | |
| # 3. Check for thrashing (e.g., > threshold evictions in window) | |
| if len(self._eviction_timestamps) > CACHE_THRASH_THRESHOLD: | |
| # 4. Rate limit the warning | |
| if now - self._last_cache_warning_time > CACHE_WARNING_COOLDOWN_SECS: | |
| self._last_cache_warning_time = now | |
| self._has_warned_cache_full = True | |
| # Format usage info | |
| used_gb = self.image_cache.currsize / (1024**3) | |
| max_gb = self.image_cache.max_bytes / (1024**3) | |
| msg = f"Cache thrashing! {len(self._eviction_timestamps)} evictions in {CACHE_THRASH_WINDOW_SECS}s. Usage: {used_gb:.1f}GB / {max_gb:.1f}GB." | |
| # Use QTimer.singleShot to ensure this runs on the main thread | |
| QTimer.singleShot(0, lambda: self.update_status_message(msg)) | |
| log.warning(msg) | |
| with self._eviction_lock: | |
| # 1. Record eviction timestamp / prune | |
| self._eviction_timestamps.append(now) | |
| cutoff = now - CACHE_THRASH_WINDOW_SECS | |
| self._eviction_timestamps = [t for t in self._eviction_timestamps if t > cutoff] | |
| # 2. Check for thrashing (e.g., > threshold evictions in window) | |
| if len(self._eviction_timestamps) > CACHE_THRASH_THRESHOLD: | |
| # 3. Rate limit the warning | |
| if now - self._last_cache_warning_time > CACHE_WARNING_COOLDOWN_SECS: | |
| self._last_cache_warning_time = now | |
| self._has_warned_cache_full = True | |
| # UI update logic | |
| used_gb = self.image_cache.currsize / (1024**3) | |
| max_gb = self.image_cache.max_bytes / (1024**3) | |
| msg = f"Cache thrashing! {len(self._eviction_timestamps)} evictions in {CACHE_THRASH_WINDOW_SECS}s. Usage: {used_gb:.1f}GB / {max_gb:.1f}GB." | |
| # Schedule UI work safely on main thread | |
| # QTimer.singleShot(0, ...) is thread-safe entry to main loop | |
| QTimer.singleShot(0, lambda: self.update_status_message(msg)) | |
| log.warning(msg) | |
| def _on_cache_evict(self, _key, _value): | |
| """Callback for when the image cache evicts an item.""" | |
| now = time.time() | |
| with self._eviction_lock: | |
| # 1. Record eviction timestamp / prune | |
| self._eviction_timestamps.append(now) | |
| cutoff = now - CACHE_THRASH_WINDOW_SECS | |
| self._eviction_timestamps = [t for t in self._eviction_timestamps if t > cutoff] | |
| # 2. Check for thrashing (e.g., > threshold evictions in window) | |
| if len(self._eviction_timestamps) > CACHE_THRASH_THRESHOLD: | |
| # 3. Rate limit the warning | |
| if now - self._last_cache_warning_time > CACHE_WARNING_COOLDOWN_SECS: | |
| self._last_cache_warning_time = now | |
| self._has_warned_cache_full = True | |
| # UI update logic | |
| used_gb = self.image_cache.currsize / (1024**3) | |
| max_gb = self.image_cache.max_bytes / (1024**3) | |
| msg = f"Cache thrashing! {len(self._eviction_timestamps)} evictions in {CACHE_THRASH_WINDOW_SECS}s. Usage: {used_gb:.1f}GB / {max_gb:.1f}GB." | |
| # Schedule UI work safely on main thread | |
| # QTimer.singleShot(0, ...) is thread-safe entry to main loop | |
| QTimer.singleShot(0, lambda: self.update_status_message(msg)) | |
| log.warning(msg) |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 4738-4738: Unused method argument: key
(ARG002)
[warning] 4738-4738: Unused method argument: value
(ARG002)
🤖 Prompt for AI Agents
In `@faststack/app.py` around lines 4738 - 4763, The _on_cache_evict callback
currently declares parameters key and value but doesn't use them, triggering a
lint error; fix by either renaming the parameters to _key and _value in the
_on_cache_evict signature to mark them intentionally unused, or incorporate them
into the warning/log and UI update (e.g., include key or a short value summary
in the msg passed to self.update_status_message and log.warning); adjust
references inside _on_cache_evict (and the lambda to self.update_status_message)
accordingly so the chosen approach is consistent.
| global _debug_mode, _debug_thumb_timing | ||
| _debug_mode = debug | ||
| _debug_thumb_timing = debug_thumb_timing | ||
|
|
There was a problem hiding this comment.
_debug_thumb_trace global is never updated.
If any code relies on that global, it will stay False even when --debug-thumbtrace is passed. Consider assigning it alongside _debug_thumb_timing.
🔧 Proposed fix
- global _debug_mode, _debug_thumb_timing
+ global _debug_mode, _debug_thumb_timing, _debug_thumb_trace
@@
_debug_mode = debug
_debug_thumb_timing = debug_thumb_timing
+ _debug_thumb_trace = debug_thumb_trace📝 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.
| global _debug_mode, _debug_thumb_timing | |
| _debug_mode = debug | |
| _debug_thumb_timing = debug_thumb_timing | |
| global _debug_mode, _debug_thumb_timing, _debug_thumb_trace | |
| _debug_mode = debug | |
| _debug_thumb_timing = debug_thumb_timing | |
| _debug_thumb_trace = debug_thumb_trace |
🤖 Prompt for AI Agents
In `@faststack/app.py` around lines 6908 - 6911, The global _debug_thumb_trace is
declared but never updated, so when the CLI flag --debug-thumbtrace is used the
module-level _debug_thumb_trace remains False; update the same block that sets
_debug_mode and _debug_thumb_timing to also set _debug_thumb_trace =
debug_thumb_trace (declared as global at top of that block) so the flag value is
propagated to the module state; ensure you reference the existing globals
_debug_mode, _debug_thumb_timing, and _debug_thumb_trace and use the incoming
parameter name debug_thumb_trace.
| with ThreadPoolExecutor(max_workers=1, initializer=set_daemon) as executor: | ||
| print(f"Result: {executor.submit(check_daemon).result()}") |
There was a problem hiding this comment.
Module-level side effect: executor runs on import.
Lines 14–15 execute immediately when this module is imported (not just when run as a script). If anything in the package accidentally imports check_daemon, a ThreadPoolExecutor will spin up as a side effect.
Wrap in a __main__ guard like the sibling script repro_cache_lock.py does:
Proposed fix
-with ThreadPoolExecutor(max_workers=1, initializer=set_daemon) as executor:
- print(f"Result: {executor.submit(check_daemon).result()}")
+if __name__ == "__main__":
+ with ThreadPoolExecutor(max_workers=1, initializer=set_daemon) as executor:
+ print(f"Result: {executor.submit(check_daemon).result()}")📝 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.
| with ThreadPoolExecutor(max_workers=1, initializer=set_daemon) as executor: | |
| print(f"Result: {executor.submit(check_daemon).result()}") | |
| if __name__ == "__main__": | |
| with ThreadPoolExecutor(max_workers=1, initializer=set_daemon) as executor: | |
| print(f"Result: {executor.submit(check_daemon).result()}") |
🤖 Prompt for AI Agents
In `@faststack/check_daemon.py` around lines 14 - 15, The module currently creates
a ThreadPoolExecutor and submits check_daemon at import time (using
ThreadPoolExecutor, set_daemon, and check_daemon); move that runtime behavior
into a script entry point by wrapping the executor block in an if __name__ ==
"__main__": guard so the ThreadPoolExecutor(max_workers=1,
initializer=set_daemon) creation and executor.submit(check_daemon).result() call
only run when the file is executed as a script, preserving the same print
behavior but preventing side effects on import.
|
|
||
| import os | ||
| from pathlib import Path | ||
|
|
||
| def norm_path(p: Path) -> str: | ||
| return os.path.normcase(os.path.abspath(str(p))) | ||
|
|
||
| p1 = Path("C:/Test/File.JPG") | ||
| p2 = Path("C:/Test/file.jpg") | ||
|
|
||
| print(f"p1: {p1}") | ||
| print(f"p2: {p2}") | ||
| print(f"p1 == p2: {p1 == p2}") | ||
|
|
||
| n1 = Path(norm_path(p1)) | ||
| n2 = Path(norm_path(p2)) | ||
|
|
||
| print(f"n1: {n1}") | ||
| print(f"n2: {n2}") | ||
| print(f"n1 == n2: {n1 == n2}") | ||
| print(f"n1 == p1: {n1 == p1}") | ||
|
|
There was a problem hiding this comment.
Debug script committed by accident — should be removed.
This file duplicates norm_path from faststack/io/variants.py and contains module-level print statements with hardcoded test paths that execute on import. It appears to be a local debugging artifact and shouldn't ship in a release PR.
🤖 Prompt for AI Agents
In `@faststack/debug_path_norm.py` around lines 1 - 22, This debug script contains
a duplicate norm_path function and module-level print statements that run on
import; remove this file (faststack/debug_path_norm.py) from the PR or at
minimum delete the debugging code: eliminate the duplicate norm_path definition
and all top-level print/test code so the module has no side effects on import;
if you want to keep the behavior, move the example into a proper unit test or a
__main__ guarded block (if truly needed) and ensure only the canonical norm_path
implementation remains in its original module (norm_path symbol in
faststack/io/variants.py).
| def get_decoded_image_size(item) -> int: | ||
| """Calculates the size of a DecodedImage object.""" | ||
| # In this simplified example, we only store the buffer. | ||
| # In the full app, this would also account for the QImage/QTexture. | ||
| from faststack.models import DecodedImage | ||
|
|
||
| if isinstance(item, DecodedImage): | ||
| # Handle both numpy arrays and memoryview buffers | ||
| if hasattr(item.buffer, "nbytes"): | ||
| return item.buffer.nbytes | ||
| elif isinstance(item.buffer, (bytes, bytearray)): | ||
| return len(item.buffer) | ||
| else: | ||
| # Fallback: estimate from dimensions (more accurate for image buffers than sys.getsizeof) | ||
| bytes_per_pixel = getattr(item, "channels", 4) # Default to RGBA | ||
| return item.width * item.height * bytes_per_pixel | ||
|
|
||
| log.warning( | ||
| f"Unexpected item type in cache: {type(item)}. Returning estimated size of 1." | ||
| ) | ||
| return 1 # Should not happen |
There was a problem hiding this comment.
get_decoded_image_size: fallback channels attribute doesn't exist on DecodedImage.
Line 27: getattr(item, "channels", 4) will always return the default 4 because DecodedImage (in faststack/models.py, lines 87–97) has no channels attribute. This makes the fallback estimate always assume RGBA, which may over-count for RGB images. Consider deriving channels from bytes_per_line / width if available.
Proposed fix
- bytes_per_pixel = getattr(item, "channels", 4) # Default to RGBA
+ bytes_per_pixel = item.bytes_per_line // item.width if item.width > 0 else 4🤖 Prompt for AI Agents
In `@faststack/imaging/cache.py` around lines 13 - 33, The fallback in
get_decoded_image_size incorrectly assumes 4 channels because DecodedImage has
no channels attribute; update the fallback to compute bytes_per_pixel from
available image metadata: if item has bytes_per_line (or a similar attribute)
and width, derive bytes_per_pixel = bytes_per_line / width and use that (rounded
or cast to int) as channels/bytes-per-pixel, otherwise fall back to 4; adjust
the branch inside get_decoded_image_size that currently uses getattr(item,
"channels", 4) to first check for item.bytes_per_line and item.width and compute
channels from that before defaulting.
|
|
||
| # Default mock behavior | ||
| self.model.rowCount.return_value = 5000 | ||
| self.model.thumbnail_size = (256, 256) |
There was a problem hiding this comment.
thumbnail_size should be int, not a tuple.
The real ThumbnailModel.thumbnail_size property returns an int (see faststack/thumbnail_view/model.py lines 161-163). Setting it to (256, 256) here won't break the test (since prefetcher.submit is mocked), but it masks any code that expects an int.
Proposed fix
- self.model.thumbnail_size = (256, 256)
+ self.model.thumbnail_size = 256📝 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.
| self.model.thumbnail_size = (256, 256) | |
| self.model.thumbnail_size = 256 |
🤖 Prompt for AI Agents
In `@faststack/tests/test_ui_prefetch_safety.py` at line 29, The test assigns
self.model.thumbnail_size = (256, 256) but ThumbnailModel.thumbnail_size is an
int; change the assignment in the test to an integer (e.g., 256) so the test
matches the real property type and surfaces any type-dependent logic (refer to
ThumbnailModel.thumbnail_size and to the test's use of prefetcher.submit which
is mocked).
| def test_main_selection_priority(self): | ||
| """Verify Main selection priority: Non-dev > Dev > Backup.""" | ||
|
|
||
| # Cas 1: Pure Main exists |
There was a problem hiding this comment.
Typo in comment: "Cas 1" → "Case 1".
Proposed fix
- # Cas 1: Pure Main exists
+ # Case 1: Pure Main exists📝 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.
| # Cas 1: Pure Main exists | |
| # Case 1: Pure Main exists |
🤖 Prompt for AI Agents
In `@faststack/tests/test_variants_logic.py` at line 10, The comment "Cas 1: Pure
Main exists" contains a typo; update that comment in
faststack/tests/test_variants_logic.py to read "Case 1: Pure Main exists"
(replace "Cas 1" with "Case 1") so the test comment is spelled correctly.
|
|
||
| # One-shot reason for logging | ||
| self._next_source_reason: Optional[str] = None | ||
|
|
||
| # Mapping from thumbnail_id (without query params) to row index | ||
| # id format: "{size}/{path_hash}/{mtime_ns}" | ||
| self._id_to_row: Dict[str, int] = {} | ||
|
|
||
| # One-shot reason for logging | ||
| self._next_source_reason: Optional[str] = None |
There was a problem hiding this comment.
Duplicate initialization of _next_source_reason.
_next_source_reason is initialized twice — at line 149 and again at line 156. This appears to be an accidental duplication from a merge or edit. The second assignment silently overwrites the first.
Remove the duplicate block
self._active_filter_flags: list = [] # current flag filters (e.g. ["uploaded", "stacked"])
-
- # One-shot reason for logging
- self._next_source_reason: Optional[str] = None
# Mapping from thumbnail_id (without query params) to row index
# id format: "{size}/{path_hash}/{mtime_ns}"
self._id_to_row: Dict[str, int] = {}
# One-shot reason for logging
self._next_source_reason: Optional[str] = None🤖 Prompt for AI Agents
In `@faststack/thumbnail_view/model.py` around lines 147 - 156, Remove the
duplicate initialization of the instance variable _next_source_reason: locate
the duplicate assignments to _next_source_reason (both set to Optional[str] =
None) in the class initializer and delete the redundant second one so the
variable is only initialized once; ensure _id_to_row and surrounding comments
remain unchanged and run tests/lint to confirm no other references relied on the
duplicate.
| @Property(str, notify=variantSaveHintChanged) | ||
| def variantSaveHint(self): | ||
| """Returns a hint message when saving from a variant.""" | ||
| return self.app_controller.get_variant_save_hint() |
There was a problem hiding this comment.
variantSaveHint lacks a hasattr guard, unlike sibling variant properties.
variantBadges (line 1331) guards with hasattr(self.app_controller, "get_variant_badges"), but variantSaveHint calls self.app_controller.get_variant_save_hint() unconditionally. This will raise AttributeError if the method is absent.
Proposed fix
`@Property`(str, notify=variantSaveHintChanged)
def variantSaveHint(self):
"""Returns a hint message when saving from a variant."""
- return self.app_controller.get_variant_save_hint()
+ if hasattr(self.app_controller, "get_variant_save_hint"):
+ return self.app_controller.get_variant_save_hint()
+ return ""🤖 Prompt for AI Agents
In `@faststack/ui/provider.py` around lines 559 - 562, The variantSaveHint
property calls self.app_controller.get_variant_save_hint() unguarded and can
raise AttributeError; update the variantSaveHint getter to check
hasattr(self.app_controller, "get_variant_save_hint") and return a safe default
(e.g., "" or None) when missing, mirroring the pattern used by variantBadges;
ensure you keep the `@Property` decorator and notify=variantSaveHintChanged and
only call get_variant_save_hint() when the attribute exists.
| - **Crop:** Added the ability to crop and rotate images via the cr(O)p hotkey (or right mouse click). It can be a freeform crop, or constrained to several popular aspect ratios. | ||
| - **Zoom & Pan:** Smooth zooming and panning. | ||
| - **Stack Selection:** Group images into stacks (`[`, `]`) and select them for processing (`S`). | ||
| - **Speak Line**: In grid view, a spark line is visible on each folder, so you can see how far you have gotten in uploading photos in each directory. |
There was a problem hiding this comment.
Typo: "Speak Line" should be "Spark Line".
The AI summary and PR description reference a "spark line" progress visualization, not "speak line."
-- **Speak Line**: In grid view, a spark line is visible on each folder, so you can see how far you have gotten in uploading photos in each directory.
+- **Spark Line**: In grid view, a spark line is visible on each folder, so you can see how far you have gotten in uploading photos in each directory.📝 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.
| - **Speak Line**: In grid view, a spark line is visible on each folder, so you can see how far you have gotten in uploading photos in each directory. | |
| - **Spark Line**: In grid view, a spark line is visible on each folder, so you can see how far you have gotten in uploading photos in each directory. |
🤖 Prompt for AI Agents
In `@README.md` at line 15, Replace the incorrect phrase "Speak Line" with "Spark
Line" wherever it appears (starting with the README entry that currently reads
"**Speak Line**: In grid view...") and also update any references in the PR
description or AI summary to use "Spark Line" so the terminology is consistent
across docs.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Dependencies