Prevent read-only metadata lookups from creating sidecar entries#48
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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)
WalkthroughSidecarManager.get_metadata gained a keyword-only create: bool parameter. Multiple read paths in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
faststack/app.py (2)
2123-2131:⚠️ Potential issue | 🟠 MajorHandle dict-backed metadata before building the badge map.
get_metadata()now returns existing entries unchanged, so a dict can still reach this method. In that case everygetattr(meta, "...", False)here falls back toFalseinstead of reading the stored key, which will clear thumbnail badges for existing metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 2123 - 2131, The badge map building uses getattr(...) which fails for dict-backed metadata returned by self.sidecar.get_metadata(stem, create=False); change the logic to detect mapping-backed metadata (e.g., isinstance(meta, dict) or collections.abc.Mapping) and read flags with meta.get("stacked", False)/meta.get("uploaded", False)/etc., otherwise fall back to getattr(meta, "stacked", False) for object-backed metadata so existing dict entries are preserved; update the code that constructs the returned dict (the block after get_metadata) to use this branching approach.
2354-2394:⚠️ Potential issue | 🟠 MajorNormalize metadata reads here too.
These paths still dereference
metaas an object, butget_metadata()returns existing entries unchanged. Ifsidecar.data.entriescontains dict-backed metadata,get_current_metadata(),add_favorites_to_batch(), andis_stacked()will raiseAttributeErrorinstead of staying read-only. Please reuse the same dict/object normalization pattern already used injump_to_last_uploaded()andadd_uploaded_to_batch().Also applies to: 2615-2616, 7377-7378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 2354 - 2394, The code assumes meta is an attribute-based object but sidecar.get_metadata() can return dicts; normalize meta to a mapping before dereferencing so attribute access becomes safe (same approach used in jump_to_last_uploaded() and add_uploaded_to_batch()). Replace the direct uses of meta (in the block building self._metadata_cache and nearby spots) by first doing meta_raw = self.sidecar.get_metadata(stem, create=False) then normalize it to a dict-like object (e.g., if isinstance(meta_raw, dict) use it, else build a dict by reading stacked, stacked_date, uploaded, uploaded_date, edited, edited_date, restacked, restacked_date, favorite, todo, todo_date via getattr) and use that normalized mapping for the keys used by get_current_metadata(), add_favorites_to_batch(), is_stacked(), and the metadata_cache assembly so AttributeError is avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@faststack/app.py`:
- Around line 2123-2131: The badge map building uses getattr(...) which fails
for dict-backed metadata returned by self.sidecar.get_metadata(stem,
create=False); change the logic to detect mapping-backed metadata (e.g.,
isinstance(meta, dict) or collections.abc.Mapping) and read flags with
meta.get("stacked", False)/meta.get("uploaded", False)/etc., otherwise fall back
to getattr(meta, "stacked", False) for object-backed metadata so existing dict
entries are preserved; update the code that constructs the returned dict (the
block after get_metadata) to use this branching approach.
- Around line 2354-2394: The code assumes meta is an attribute-based object but
sidecar.get_metadata() can return dicts; normalize meta to a mapping before
dereferencing so attribute access becomes safe (same approach used in
jump_to_last_uploaded() and add_uploaded_to_batch()). Replace the direct uses of
meta (in the block building self._metadata_cache and nearby spots) by first
doing meta_raw = self.sidecar.get_metadata(stem, create=False) then normalize it
to a dict-like object (e.g., if isinstance(meta_raw, dict) use it, else build a
dict by reading stacked, stacked_date, uploaded, uploaded_date, edited,
edited_date, restacked, restacked_date, favorite, todo, todo_date via getattr)
and use that normalized mapping for the keys used by get_current_metadata(),
add_favorites_to_batch(), is_stacked(), and the metadata_cache assembly so
AttributeError is avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbc63649-f820-421a-bac8-3e91cba46883
📒 Files selected for processing (2)
faststack/app.pyfaststack/io/sidecar.py
|
Good catch. I’ll keep this to a small follow-up and align those remaining read-only paths with the existing normalization pattern. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
faststack/app.py (1)
1801-1804: Drop the unreachabledictbranches aroundget_metadata().
SidecarManager.get_metadata()returns anEntryMetadatadataclass even whencreate=False, so theseisinstance(meta, dict)paths—and theif not metaguard on Line 2671—never fire for the real sidecar implementation. Keeping both shapes here makes the caller contract harder to follow and can hide normalization regressions at the sidecar boundary.faststack/io/sidecar.py:121-128andfaststack/models.py:61-77both point to a single object shape.Also applies to: 2381-2391, 2616-2616, 2671-2677, 7378-7378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 1801 - 1804, The code is handling two shapes for metadata but SidecarManager.get_metadata()/EntryMetadata always returns the dataclass shape, so remove the isinstance(meta, dict) branches and the redundant falsy checks and instead treat meta as EntryMetadata: access attributes directly (e.g., use meta.uploaded or getattr(meta, "uploaded", None) if you need a safe fallback) in the blocks that reference uploaded and similar fields; update all affected call sites that use the variable named meta (including the locations that currently check isinstance(meta, dict)) to assume the single EntryMetadata shape and drop the dict-branch normalization to keep the caller contract consistent with SidecarManager.get_metadata()/EntryMetadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@faststack/app.py`:
- Around line 1801-1804: The code is handling two shapes for metadata but
SidecarManager.get_metadata()/EntryMetadata always returns the dataclass shape,
so remove the isinstance(meta, dict) branches and the redundant falsy checks and
instead treat meta as EntryMetadata: access attributes directly (e.g., use
meta.uploaded or getattr(meta, "uploaded", None) if you need a safe fallback) in
the blocks that reference uploaded and similar fields; update all affected call
sites that use the variable named meta (including the locations that currently
check isinstance(meta, dict)) to assume the single EntryMetadata shape and drop
the dict-branch normalization to keep the caller contract consistent with
SidecarManager.get_metadata()/EntryMetadata.
|
Fixed. I removed the unreachable dict branches around get_metadata() in the affected read-only paths and dropped the redundant guard in add_uploaded_to_batch() to match the current SidecarManager contract. |
There was a problem hiding this comment.
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)
2348-2385:⚠️ Potential issue | 🟠 MajorGuard
todoreads for older metadata objects.Line 2384 and Line 2385 now assume every
EntryMetadatahastodo/todo_date, but the rest of this file still treats those fields as optional. Older sidecar entries can now blow upget_current_metadata()on a pure read.Suggested fix
self._metadata_cache = { "filename": filename, "exif_brief": exif_brief, "stacked": meta.stacked, "stacked_date": meta.stacked_date or "", "uploaded": meta.uploaded, "uploaded_date": meta.uploaded_date or "", "edited": meta.edited, "edited_date": meta.edited_date or "", "restacked": meta.restacked, "restacked_date": meta.restacked_date or "", "favorite": meta.favorite, - "todo": meta.todo, - "todo_date": meta.todo_date or "", + "todo": getattr(meta, "todo", False), + "todo_date": getattr(meta, "todo_date", None) or "", "stack_info_text": stack_info, "batch_info_text": batch_info, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 2348 - 2385, get_current_metadata is assuming EntryMetadata always has todo and todo_date which can raise on older sidecar entries; change the reads where meta.todo and meta.todo_date are used (in the block that populates self._metadata_cache) to safely access optional attributes, e.g. use getattr(meta, "todo", False) and getattr(meta, "todo_date", "") (or an equivalent hasattr-based guard) so older metadata without those fields yields sensible defaults and doesn't crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@faststack/app.py`:
- Around line 2348-2385: get_current_metadata is assuming EntryMetadata always
has todo and todo_date which can raise on older sidecar entries; change the
reads where meta.todo and meta.todo_date are used (in the block that populates
self._metadata_cache) to safely access optional attributes, e.g. use
getattr(meta, "todo", False) and getattr(meta, "todo_date", "") (or an
equivalent hasattr-based guard) so older metadata without those fields yields
sensible defaults and doesn't crash.
|
Good catch. I’m currently rate-limited on my tooling, but I’ll update the todo and todo_date reads in get_current_metadata() to use safe getattr defaults shortly. |
2 participants
Audited all get_metadata() call sites and updated the pure read paths in faststack/app.py to use create=False, so UI-only reads like folder badges, current metadata display, last-uploaded navigation, batch helpers, and stacked-status checks no longer create persistent sidecar entries. Verified that all mutation paths still use the default creating behavior for upload, favorite, stacked, restack, edit, drag/upload, and Helicon save flows.
Summary by CodeRabbit