Pr/optimize sidecar#83
Merged
AlanRockefeller merged 2 commits intomainfrom Apr 17, 2026
Merged
Conversation
WalkthroughUpdated metadata keying strategy from sidecar-derived stable keys to Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Controller
participant Sidecar as SidecarManager
participant TView as ThumbnailView
rect rgba(100, 150, 200, 0.5)
Note over App,TView: Bulk Metadata Retrieval with Per-Image Error Isolation
App->>App: _get_bulk_metadata_map()
loop For each image
App->>Sidecar: get_metadata(img.path)
alt Metadata retrieved
Sidecar-->>App: metadata
App->>App: metadata_map[str(img.path)] = metadata
else Error on get_metadata
Sidecar-->>App: Exception
App->>App: Log warning, continue
end
end
App->>TView: refresh_from_controller(metadata_map)
end
rect rgba(200, 150, 100, 0.5)
Note over Sidecar: Cached Key Computation
TView->>Sidecar: metadata_key_for_path(img.path)
alt Cache hit
Sidecar->>Sidecar: Return cached result
Sidecar-->>TView: stable_key
else Cache miss
Sidecar->>Sidecar: Compute relative path, store in cache
Sidecar-->>TView: stable_key
end
end
rect rgba(150, 200, 150, 0.5)
Note over TView: Metadata Lookup by Path
TView->>TView: For each image, lookup str(img.path) in metadata_map
TView->>TView: Apply flags (favorite, uploaded, stacked)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
faststack/io/sidecar.py (1)
221-223: Eviction is FIFO, not LRU — fine for now, but note the pathological case.
del self._stable_key_cache[next(iter(self._stable_key_cache))]evicts the oldest-inserted entry (insertion order), not the least-recently-used, because cache hits don't reinsert the key. For the documented workload (bulk-map build + flag filter + grid construction over a single folder), total unique paths are typically well below_key_cache_max = 8192, so this never matters.If a folder ever exceeds 8192 images and the same paths are re-queried across passes, you can get thrash where hot keys get evicted in favor of one-shot cold keys. Two cheap options if you want to harden this:
♻️ Optional: promote on hit to get LRU semantics
- cached = self._stable_key_cache.get(cache_key) - if cached is not None: - return cached + cached = self._stable_key_cache.get(cache_key) + if cached is not None: + # Move to end for LRU eviction + self._stable_key_cache[cache_key] = self._stable_key_cache.pop(cache_key) + return cachedOr simply swap both caches for
functools.lru_cache-backed helpers / a smallOrderedDictwrapper.Not blocking — flagging as a nice-to-have.
Also applies to: 271-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/io/sidecar.py` around lines 221 - 223, The cache eviction currently deletes the first inserted key via del self._stable_key_cache[next(iter(self._stable_key_cache))], producing FIFO instead of LRU behavior; to fix, make the cache LRU by promoting a hit before reinserting (or use an OrderedDict/ functools.lru_cache-backed helper): when accessing self._stable_key_cache for cache_key, move that entry to the "most-recent" end (e.g., via move_to_end) so subsequent evictions remove true least-recently-used items, and keep the existing eviction logic using self._key_cache_max; apply the same change to the other symmetric cache usage referenced (the similar block at the other location).
🤖 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/app.py`:
- Around line 2611-2615: In AppController._get_bulk_metadata_map, the current
mapping keys use the raw image path (e.g., str(img.path) /
normalize_path_key(img.path)) which breaks the sidecar key contract; update the
code so the dictionary keys are the canonical sidecar keys by calling
SidecarManager.metadata_key_for_path(path) (use the same path variable you
already have) when inserting into bulk_map so downstream consumers (thumbnail
model, folder stats) can reliably look up metadata.
---
Nitpick comments:
In `@faststack/io/sidecar.py`:
- Around line 221-223: The cache eviction currently deletes the first inserted
key via del self._stable_key_cache[next(iter(self._stable_key_cache))],
producing FIFO instead of LRU behavior; to fix, make the cache LRU by promoting
a hit before reinserting (or use an OrderedDict/ functools.lru_cache-backed
helper): when accessing self._stable_key_cache for cache_key, move that entry to
the "most-recent" end (e.g., via move_to_end) so subsequent evictions remove
true least-recently-used items, and keep the existing eviction logic using
self._key_cache_max; apply the same change to the other symmetric cache usage
referenced (the similar block at the other location).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d18af2ac-6484-4e43-b059-4d7226be6a07
📒 Files selected for processing (4)
faststack/app.pyfaststack/io/sidecar.pyfaststack/tests/test_bulk_metadata_map.pyfaststack/thumbnail_view/model.py
Comment on lines
+2611
to
+2615
| """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. |
There was a problem hiding this comment.
Use canonical sidecar keys here to avoid cross-consumer metadata misses.
On Line 2628, keying bulk_map by str(img.path) breaks the established contract for this method and can cause lookup mismatches where consumers rely on canonical sidecar keys.
Suggested fix
- """Get flattened metadata map for the given images, keyed by str(img.path).
+ """Get flattened metadata map for the given images, keyed by sidecar canonical key.
@@
- bulk_map[str(img.path)] = {
+ key = self.sidecar.metadata_key_for_path(img.path)
+ bulk_map[key] = {
"stacked": getattr(meta, "stacked", False),
"uploaded": getattr(meta, "uploaded", False),
"edited": getattr(meta, "edited", False),
"restacked": getattr(meta, "restacked", False),
"favorite": getattr(meta, "favorite", False),
"todo": getattr(meta, "todo", False),
}Based on learnings: In faststack.app.AppController._get_bulk_metadata_map, the dictionary must be keyed by SidecarManager.metadata_key_for_path(path) (the sidecar canonical key), not by normalize_path_key(img.path), to avoid metadata lookup misses across consumers like the thumbnail model and folder stats.
Also applies to: 2628-2628
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/app.py` around lines 2611 - 2615, In
AppController._get_bulk_metadata_map, the current mapping keys use the raw image
path (e.g., str(img.path) / normalize_path_key(img.path)) which breaks the
sidecar key contract; update the code so the dictionary keys are the canonical
sidecar keys by calling SidecarManager.metadata_key_for_path(path) (use the same
path variable you already have) when inserting into bulk_map so downstream
consumers (thumbnail model, folder stats) can reliably look up metadata.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Optimize bulk metadata refresh and isolate per-image sidecar failures
Summary by CodeRabbit
Bug Fixes
Performance
Tests