diff --git a/faststack/app.py b/faststack/app.py index 127d7a3..e57bbd0 100644 --- a/faststack/app.py +++ b/faststack/app.py @@ -2608,18 +2608,24 @@ def _get_metadata_dict(self, image_path: Path | str) -> dict: } def _get_bulk_metadata_map(self, images=None) -> Dict[str, dict]: - """Get flattened metadata map for the given images (defaults to self.image_files). + """Get flattened metadata map for the given images, keyed by str(img.path). - Used to avoid per-image sidecar lookups on the UI thread during grid refresh. + Used to avoid per-image sidecar lookups on the UI thread during grid + refresh. Keying by the raw path string lets downstream consumers + look up entries directly without re-resolving the stable sidecar key. + + The map is treated as authoritative by the grid (see + ``ThumbnailModel.refresh_from_controller``), so per-image errors must + be isolated — a single bad path must not collapse flags for the + whole folder. """ bulk_map = {} - try: - for img in (images if images is not None else self.image_files): - key = self.sidecar.metadata_key_for_path(img.path) + for img in (images if images is not None else self.image_files): + try: meta = self.sidecar.get_metadata(img.path, create=False) if meta is None: continue - bulk_map[key] = { + bulk_map[str(img.path)] = { "stacked": getattr(meta, "stacked", False), "uploaded": getattr(meta, "uploaded", False), "edited": getattr(meta, "edited", False), @@ -2627,8 +2633,12 @@ def _get_bulk_metadata_map(self, images=None) -> Dict[str, dict]: "favorite": getattr(meta, "favorite", False), "todo": getattr(meta, "todo", False), } - except Exception as e: - log.warning("Failed to build bulk metadata map: %s", e) + except Exception as e: + log.warning( + "Failed to read metadata for %s during bulk build: %s", + img.path, + e, + ) return bulk_map def _invalidate_batch_cache(self): diff --git a/faststack/io/sidecar.py b/faststack/io/sidecar.py index cb2fb92..eae64cd 100644 --- a/faststack/io/sidecar.py +++ b/faststack/io/sidecar.py @@ -46,6 +46,17 @@ def __init__(self, directory: Path, watcher, debug: bool = False): self.path = directory / "faststack.json" self.watcher = watcher self.debug = debug + # Precomputed once: the case-normalized absolute base dir used by + # metadata_key_for_path / _metadata_filename_key on every call. + self._base_dir_normcased = Path( + os.path.normcase(os.path.abspath(str(directory))) + ) + # Bounded per-instance caches: input str → resolved key. Folder + # refresh resolves the same paths repeatedly across bulk-map build, + # flag filter, and grid entry construction. + self._stable_key_cache: dict[str, str] = {} + self._filename_key_cache: dict[str, str] = {} + self._key_cache_max = 8192 self.data = self.load() def stop_watcher(self): @@ -187,21 +198,30 @@ def get_metadata( def metadata_key_for_path(self, image_path: Union[str, Path]) -> str: """Return the stable sidecar key for a concrete image path.""" + cache_key = str(image_path) + cached = self._stable_key_cache.get(cache_key) + if cached is not None: + return cached + path = Path(image_path) if not path.name: return "" if not path.is_absolute(): path = self.directory / path - base_dir = Path(os.path.normcase(os.path.abspath(str(self.directory)))) abs_path = Path(os.path.normcase(os.path.abspath(str(path)))) try: - relative = abs_path.relative_to(base_dir) + relative = abs_path.relative_to(self._base_dir_normcased) stable_path = relative.parent / relative.stem - return str(stable_path).replace("\\", "/") + result = str(stable_path).replace("\\", "/") except ValueError: stable_path = abs_path.parent / abs_path.stem - return str(stable_path).replace("\\", "/") + result = str(stable_path).replace("\\", "/") + + if len(self._stable_key_cache) >= self._key_cache_max: + del self._stable_key_cache[next(iter(self._stable_key_cache))] + self._stable_key_cache[cache_key] = result + return result def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]: """Return (stable_key, migration_candidate_keys) for a metadata lookup.""" @@ -230,19 +250,28 @@ def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]: def _metadata_filename_key(self, image_path: Union[str, Path]) -> str: """Return the extension-preserving key used by the regressed patch.""" + cache_key = str(image_path) + cached = self._filename_key_cache.get(cache_key) + if cached is not None: + return cached + path = Path(image_path) if not path.name: return "" if not path.is_absolute(): path = self.directory / path - base_dir = Path(os.path.normcase(os.path.abspath(str(self.directory)))) abs_path = Path(os.path.normcase(os.path.abspath(str(path)))) try: - relative = abs_path.relative_to(base_dir) - return str(relative).replace("\\", "/") + relative = abs_path.relative_to(self._base_dir_normcased) + result = str(relative).replace("\\", "/") except ValueError: - return str(abs_path).replace("\\", "/") + result = str(abs_path).replace("\\", "/") + + if len(self._filename_key_cache) >= self._key_cache_max: + del self._filename_key_cache[next(iter(self._filename_key_cache))] + self._filename_key_cache[cache_key] = result + return result def _stable_key_from_key(self, key: str, check_fs: bool = False) -> str: """Convert any historical sidecar key form into today's stable key. diff --git a/faststack/tests/test_bulk_metadata_map.py b/faststack/tests/test_bulk_metadata_map.py new file mode 100644 index 0000000..0f1bbb0 --- /dev/null +++ b/faststack/tests/test_bulk_metadata_map.py @@ -0,0 +1,77 @@ +"""Tests for AppController._get_bulk_metadata_map error isolation. + +The grid (ThumbnailModel.refresh_from_controller) treats a non-None metadata +map as authoritative — it does not fall back to per-image sidecar lookups on +miss. So per-image errors during bulk build must be isolated; one bad path +must not silently force every grid flag to false for the whole folder. +""" + +from faststack.models import EntryMetadata, ImageFile + + +def _make_image(tmp_path, name): + p = tmp_path / name + p.write_bytes(b"\xff\xd8") + return ImageFile(path=p, timestamp=1000.0) + + +def test_per_image_error_does_not_invalidate_other_entries(app_controller, tmp_path): + """One sidecar.get_metadata raise must not drop other images from the map.""" + a = _make_image(tmp_path, "a.jpg") + bad = _make_image(tmp_path, "b.jpg") + c = _make_image(tmp_path, "c.jpg") + app_controller.image_files = [a, bad, c] + + def fake_get_metadata(path, *, create): + if path == bad.path: + raise OSError("simulated sidecar failure for one image") + return EntryMetadata(favorite=True, uploaded=True) + + app_controller.sidecar.get_metadata.side_effect = fake_get_metadata + + bulk_map = app_controller._get_bulk_metadata_map() + + assert str(a.path) in bulk_map + assert str(c.path) in bulk_map + assert str(bad.path) not in bulk_map + assert bulk_map[str(a.path)]["favorite"] is True + assert bulk_map[str(a.path)]["uploaded"] is True + assert bulk_map[str(c.path)]["favorite"] is True + + +def test_all_images_raise_yields_empty_map(app_controller, tmp_path): + """Documents the contract: when every lookup fails, the map is empty. + + The grid treats this empty map as authoritative — the resulting all-false + flags are intentional, not a silent regression. Any future change to the + error-path contract should update this test. + """ + a = _make_image(tmp_path, "a.jpg") + b = _make_image(tmp_path, "b.jpg") + app_controller.image_files = [a, b] + + app_controller.sidecar.get_metadata.side_effect = OSError("everything is broken") + + bulk_map = app_controller._get_bulk_metadata_map() + + assert bulk_map == {} + + +def test_first_image_raises_subsequent_succeed(app_controller, tmp_path): + """Earlier exceptions must not skip later images (regression for outer try).""" + bad = _make_image(tmp_path, "a.jpg") + good = _make_image(tmp_path, "b.jpg") + app_controller.image_files = [bad, good] + + def fake_get_metadata(path, *, create): + if path == bad.path: + raise RuntimeError("first one fails") + return EntryMetadata(stacked=True) + + app_controller.sidecar.get_metadata.side_effect = fake_get_metadata + + bulk_map = app_controller._get_bulk_metadata_map() + + assert str(bad.path) not in bulk_map + assert str(good.path) in bulk_map + assert bulk_map[str(good.path)]["stacked"] is True diff --git a/faststack/thumbnail_view/model.py b/faststack/thumbnail_view/model.py index 6096d71..787619b 100644 --- a/faststack/thumbnail_view/model.py +++ b/faststack/thumbnail_view/model.py @@ -572,10 +572,15 @@ def refresh_from_controller( if self._active_filter_flags: flags = self._active_filter_flags filtered = [] + # When metadata_map is provided (even empty) treat it as + # authoritative — don't fall through to per-image sidecar + # lookups, and key lookups by str(img.path) so we don't + # re-resolve a stable key the bulk-map builder already paid for. + map_authoritative = metadata_map is not None for img in images: try: - if metadata_map and self._metadata_key_fn: - meta = metadata_map.get(self._metadata_key_fn(img.path), {}) + if map_authoritative: + meta = metadata_map.get(str(img.path), {}) elif self._get_metadata: meta = self._get_metadata(img.path) else: @@ -618,6 +623,9 @@ def _add_images_to_entries( self, images: List, metadata_map: Optional[Dict[str, dict]] = None ): """Convert list of objects (ImageFile or similar) to ThumbnailEntry.""" + # See refresh_from_controller: a non-None metadata_map is authoritative + # and keyed by str(img.path). + map_authoritative = metadata_map is not None for img in images: try: # Use mtime from object if available to avoid stat() @@ -636,8 +644,8 @@ def _add_images_to_entries( is_favorite = False is_todo = False - if metadata_map and self._metadata_key_fn: - meta = metadata_map.get(self._metadata_key_fn(img.path), {}) + if map_authoritative: + meta = metadata_map.get(str(img.path), {}) is_stacked = meta.get("stacked", False) is_uploaded = meta.get("uploaded", False) is_edited = meta.get("edited", False)