Merge changes from PR54 + some fixes to PR54#57
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughMetadata access was changed from stem-based keys to stable path-aware keys. SidecarManager now accepts Path/str refs, normalizes/migrates legacy keys, and Sidecar lookups across app, thumbnail model, and folder stats were updated to use full paths. Logging and tests were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as "UI / Controller"
participant Model as "ThumbnailModel"
participant Sidecar as "SidecarManager"
participant FS as "Filesystem"
UI->>Model: request metadata for image (Path)
Model->>Sidecar: get_metadata(image_path)
Sidecar->>Sidecar: compute stable_key + candidate_keys
Sidecar->>Sidecar: if legacy key present -> migrate to stable_key
Sidecar->>FS: (optional) check filesystem for path normalization
Sidecar-->>Model: return EntryMetadata (create if allowed)
Model-->>UI: present metadata / update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cd43bf4ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/app.py (1)
1810-1815:⚠️ Potential issue | 🟠 MajorKeep read-side metadata access schema-tolerant.
_get_bulk_metadata_map()andtoggle_todo()already treat metadata fields as optional, but these paths still do directmeta.<field>reads. On older sidecars that predate newer fields, that can either collapse the UI to all-false metadata or raise while scanning uploaded/favorite images or rendering the metadata panel.🛠️ Minimal hardening pattern
- uploaded = meta.uploaded if meta else False + uploaded = getattr(meta, "uploaded", False) if meta else False - "todo": meta.todo, - "todo_date": (meta.todo_date or "") if meta else "", + "todo": getattr(meta, "todo", False), + "todo_date": (getattr(meta, "todo_date", "") or "") if meta else "", - if meta and meta.favorite: + if meta and getattr(meta, "favorite", False):Mirror that same
getattr(..., default)pattern for the other flag/date reads in these methods.Also applies to: 2126-2148, 2360-2401, 2621-2624, 2676-2679, 7362-7365
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 1810 - 1815, The code directly accesses metadata fields (e.g., meta.uploaded) which can break on older sidecars; change these reads to use getattr with safe defaults (for boolean flags use getattr(meta, "uploaded", False) or getattr(meta, "favorite", False), for dates use getattr(meta, "last_viewed", None) or an appropriate default) in the affected methods (_get_bulk_metadata_map, toggle_todo and the blocks around the shown diffs) so missing attributes don't raise and older schemas render correctly.
🧹 Nitpick comments (2)
faststack/logging_setup.py (1)
53-53: Consider using an absolute path for the local fallback.
Path.cwd() / "var" / "appdata"captures the working directory at call time. If the process changes its working directory before this function runs, the candidate path may resolve unexpectedly. Consider whether this relative-path candidate is intentional or if an absolute path (e.g., relative to the module/package location) would be more predictable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/logging_setup.py` at line 53, The current fallback candidate uses Path.cwd() (candidates.append(Path.cwd() / "var" / "appdata")) which depends on the process working directory; change it to build an absolute path anchored to the module/package (for example using Path(__file__).resolve().parent or the package root) so the candidate is deterministic regardless of CWD changes—update the code that appends to candidates to use that module-root based Path instead of Path.cwd().faststack/io/utils.py (1)
23-23: Centralize path-key construction on this helper.Now that
normalize_path_key()forces/separators, the remaining rawos.path.normcase(os.path.abspath(...))call sites infaststack/thumbnail_view/model.py(_get_loupe_index_for_entry()andremove_rows_by_path()) are easy to drift from the canonical form. I'd switch those to this helper too so Windows lookups and hashes stay coupled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/io/utils.py` at line 23, Normalize path-key construction by replacing raw os.path.normcase(os.path.abspath(...)).replace("\\", "/") usages with the canonical helper normalize_path_key(). Update _get_loupe_index_for_entry() and remove_rows_by_path() in thumbnail_view/model.py to call normalize_path_key(path) for all path normalization/lookup and hashing operations, and add the necessary import for normalize_path_key at the top of the module so Windows separators and casing are handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/io/sidecar.py`:
- Around line 156-179: The current lookup in sidecar.py promotes only the first
matching alias into self.data.entries[stable_key], which can drop fields from
other aliases; update the logic around the candidate_keys/stable_key resolution
(the block that reads self.data.entries.get(stable_key), loops candidate_keys,
and the fallback using _stable_key_from_key) to collect all matching
candidate_meta objects whose keys normalize to stable_key, merge their metadata
fields deterministically (or detect conflicting keys and surface an error),
remove all original alias keys from self.data.entries, and write the merged meta
once to self.data.entries[stable_key]; also add a regression test that seeds two
legacy keys (e.g., "photo.CR2" and "photo.jpg") with different fields and
asserts the merged canonical "photo" contains both sets of fields (or that a
conflict is reported).
- Around line 204-225: The _lookup_keys function currently uses path.stem as a
migration candidate for nested paths which lets a bare-stem key like "photo" be
treated as a match for Path("a/photo.jpg"); change the logic in _lookup_keys so
the bare-stem fallback (path.stem) is only returned when the path is top-level
(no parent or parent is "."/empty), both in the Path branch and the string-path
branch, and otherwise omit the bare-stem candidate for nested paths to avoid
migrating top-level metadata in get_metadata; also add a regression test that
creates a canonical "photo" metadata entry and then calls
get_metadata/_lookup_keys for Path("a/photo.jpg") asserting the nested lookup
does not migrate or return the bare "photo" entry.
In `@faststack/thumbnail_view/model.py`:
- Around line 552-555: The code treats an empty metadata_map as falsy and falls
back to per-image _get_metadata, causing bulk-refresh to be bypassed; change the
conditional to check for metadata_map is not None (or explicitly "metadata_map
is not None") instead of truthiness so that an empty dict from
_get_bulk_metadata_map() is accepted and used when _metadata_key_fn is present
(affect the blocks using metadata_map and self._metadata_key_fn, and repeat the
same change at the other occurrence around the _get_metadata usage at lines
corresponding to the second block).
---
Outside diff comments:
In `@faststack/app.py`:
- Around line 1810-1815: The code directly accesses metadata fields (e.g.,
meta.uploaded) which can break on older sidecars; change these reads to use
getattr with safe defaults (for boolean flags use getattr(meta, "uploaded",
False) or getattr(meta, "favorite", False), for dates use getattr(meta,
"last_viewed", None) or an appropriate default) in the affected methods
(_get_bulk_metadata_map, toggle_todo and the blocks around the shown diffs) so
missing attributes don't raise and older schemas render correctly.
---
Nitpick comments:
In `@faststack/io/utils.py`:
- Line 23: Normalize path-key construction by replacing raw
os.path.normcase(os.path.abspath(...)).replace("\\", "/") usages with the
canonical helper normalize_path_key(). Update _get_loupe_index_for_entry() and
remove_rows_by_path() in thumbnail_view/model.py to call
normalize_path_key(path) for all path normalization/lookup and hashing
operations, and add the necessary import for normalize_path_key at the top of
the module so Windows separators and casing are handled consistently.
In `@faststack/logging_setup.py`:
- Line 53: The current fallback candidate uses Path.cwd()
(candidates.append(Path.cwd() / "var" / "appdata")) which depends on the process
working directory; change it to build an absolute path anchored to the
module/package (for example using Path(__file__).resolve().parent or the package
root) so the candidate is deterministic regardless of CWD changes—update the
code that appends to candidates to use that module-root based Path instead of
Path.cwd().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 066240fe-7580-474f-bc96-4fd9be18f214
📒 Files selected for processing (9)
faststack/app.pyfaststack/io/sidecar.pyfaststack/io/utils.pyfaststack/logging_setup.pyfaststack/tests/test_jump_to_last_uploaded.pyfaststack/tests/test_sidecar.pyfaststack/tests/thumbnail_view/test_folder_stats.pyfaststack/thumbnail_view/folder_stats.pyfaststack/thumbnail_view/model.py
| meta = self.data.entries.get(stable_key) | ||
| if meta is None: | ||
| for candidate_key in candidate_keys: | ||
| if candidate_key == stable_key: | ||
| continue | ||
| candidate_meta = self.data.entries.get(candidate_key) | ||
| if candidate_meta is None: | ||
| continue | ||
| meta = candidate_meta | ||
| if stable_key not in self.data.entries: | ||
| self.data.entries[stable_key] = candidate_meta | ||
| if candidate_key in self.data.entries and candidate_key != stable_key: | ||
| del self.data.entries[candidate_key] | ||
| break | ||
| if meta is None: | ||
| for existing_key, existing_meta in list(self.data.entries.items()): | ||
| if existing_key == stable_key: | ||
| continue | ||
| if self._stable_key_from_key(existing_key, check_fs=True) != stable_key: | ||
| continue | ||
| meta = existing_meta | ||
| self.data.entries[stable_key] = existing_meta | ||
| del self.data.entries[existing_key] | ||
| break |
There was a problem hiding this comment.
Promoting only the first matching alias can hide metadata.
If a regressed sidecar contains both photo.CR2 and photo.jpg, this block migrates whichever alias it sees first to photo and leaves the other behind. Later lookups hit photo immediately, so fields that lived only on the skipped alias become unreachable. Please merge all aliases that normalize to the same stable_key (or surface a conflict) before finalizing the canonical entry, and add a regression that seeds both legacy keys with different fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/io/sidecar.py` around lines 156 - 179, The current lookup in
sidecar.py promotes only the first matching alias into
self.data.entries[stable_key], which can drop fields from other aliases; update
the logic around the candidate_keys/stable_key resolution (the block that reads
self.data.entries.get(stable_key), loops candidate_keys, and the fallback using
_stable_key_from_key) to collect all matching candidate_meta objects whose keys
normalize to stable_key, merge their metadata fields deterministically (or
detect conflicting keys and surface an error), remove all original alias keys
from self.data.entries, and write the merged meta once to
self.data.entries[stable_key]; also add a regression test that seeds two legacy
keys (e.g., "photo.CR2" and "photo.jpg") with different fields and asserts the
merged canonical "photo" contains both sets of fields (or that a conflict is
reported).
| def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]: | ||
| """Return (stable_key, migration_candidate_keys) for a metadata lookup.""" | ||
| if isinstance(image_ref, Path): | ||
| if not image_ref.name: | ||
| return "", [] | ||
| stable_key = self.metadata_key_for_path(image_ref) | ||
| full_name_key = self._metadata_filename_key(image_ref) | ||
| return stable_key, [full_name_key, image_ref.stem] | ||
|
|
||
| value = str(image_ref) | ||
| if not value: | ||
| return "", [] | ||
|
|
||
| # Only treat string as a path if it contains explicit path separators. | ||
| # Dotted strings (even with image extensions like "photo.CR2") are | ||
| # treated as exact keys — migration of legacy filename keys is handled | ||
| # by the _stable_key_from_key scan in get_metadata. | ||
| if os.path.sep in value or "/" in value or "\\" in value: | ||
| path = Path(value) | ||
| stable_key = self.metadata_key_for_path(path) | ||
| full_name_key = self._metadata_filename_key(path) | ||
| return stable_key, [full_name_key, path.stem] |
There was a problem hiding this comment.
Don't let nested lookups steal top-level metadata.
For Path("a/photo.jpg"), _lookup_keys() currently returns ["a/photo.jpg", "photo"]. If "photo" already exists as the canonical key for a top-level file, get_metadata() will migrate that entry to "a/photo" and the root image loses its metadata. The bare-stem fallback is only safe for top-level paths; nested paths need stronger disambiguation than path.stem.
💡 Narrow the bare-stem fallback to top-level paths
def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]:
"""Return (stable_key, migration_candidate_keys) for a metadata lookup."""
if isinstance(image_ref, Path):
if not image_ref.name:
return "", []
stable_key = self.metadata_key_for_path(image_ref)
full_name_key = self._metadata_filename_key(image_ref)
- return stable_key, [full_name_key, image_ref.stem]
+ candidate_keys = [full_name_key]
+ if "/" not in stable_key:
+ candidate_keys.append(image_ref.stem)
+ return stable_key, candidate_keys
...
if os.path.sep in value or "/" in value or "\\" in value:
path = Path(value)
stable_key = self.metadata_key_for_path(path)
full_name_key = self._metadata_filename_key(path)
- return stable_key, [full_name_key, path.stem]
+ candidate_keys = [full_name_key]
+ if "/" not in stable_key:
+ candidate_keys.append(path.stem)
+ return stable_key, candidate_keysPlease also add a regression for an existing "photo" entry plus a lookup for Path("a/photo.jpg").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/io/sidecar.py` around lines 204 - 225, The _lookup_keys function
currently uses path.stem as a migration candidate for nested paths which lets a
bare-stem key like "photo" be treated as a match for Path("a/photo.jpg"); change
the logic in _lookup_keys so the bare-stem fallback (path.stem) is only returned
when the path is top-level (no parent or parent is "."/empty), both in the Path
branch and the string-path branch, and otherwise omit the bare-stem candidate
for nested paths to avoid migrating top-level metadata in get_metadata; also add
a regression test that creates a canonical "photo" metadata entry and then calls
get_metadata/_lookup_keys for Path("a/photo.jpg") asserting the nested lookup
does not migrate or return the bare "photo" entry.
| if metadata_map and self._metadata_key_fn: | ||
| meta = metadata_map.get(self._metadata_key_fn(img.path), {}) | ||
| elif self._get_metadata: | ||
| meta = self._get_metadata(img.path.stem) | ||
| meta = self._get_metadata(img.path) |
There was a problem hiding this comment.
Treat an empty metadata_map as present.
An empty bulk map is the normal output of faststack/app.py:_get_bulk_metadata_map() when a folder has images but no sidecar flags yet. With the current truthiness check, {} falls back to _get_metadata() here, which reintroduces per-image lookups and lets the bulk-refresh path stop using a single metadata source.
🛠️ Minimal fix
- if metadata_map and self._metadata_key_fn:
+ if metadata_map is not None and self._metadata_key_fn:
meta = metadata_map.get(self._metadata_key_fn(img.path), {})
@@
- if metadata_map and self._metadata_key_fn:
+ if metadata_map is not None and self._metadata_key_fn:
meta = metadata_map.get(self._metadata_key_fn(img.path), {})Also applies to: 614-615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/thumbnail_view/model.py` around lines 552 - 555, The code treats an
empty metadata_map as falsy and falls back to per-image _get_metadata, causing
bulk-refresh to be bypassed; change the conditional to check for metadata_map is
not None (or explicitly "metadata_map is not None") instead of truthiness so
that an empty dict from _get_bulk_metadata_map() is accepted and used when
_metadata_key_fn is present (affect the blocks using metadata_map and
self._metadata_key_fn, and repeat the same change at the other occurrence around
the _get_metadata usage at lines corresponding to the second block).
|
@coderabbitai 1. Sidecar alias merging (lines 156-179) — NOT a real bug. The concern is that two legacy keys (e.g., "photo.CR2" and "photo.jpg") could both exist with different metadata, and only the first is promoted. In practice,
The concern is that path.stem ("photo") is returned as a candidate for Path("a/photo.jpg"), potentially matching a top-level "photo" entry. However,
if metadata_map and self._metadata_key_fn: treats {} as falsy and falls back to per-image _get_metadata. But if the map is empty, no images have metadata — both
EntryMetadata is a @dataclasses.dataclass with explicit defaults (uploaded: bool = False, favorite: bool = False, todo: bool = False, etc.). The
Path.cwd() / "var" / "appdata" is fallback candidate #4 of 5 (after APPDATA, LOCALAPPDATA, and ~/.faststack). On any normal system, an earlier candidate |
|
Tip For best results, initiate chat on the files or code changes.
[resolve] ✏️ Learnings added
🧠 Learnings used |
2 participants
Summary by CodeRabbit
Bug Fixes
Reliability
Tests