Implement PR54 + fixes#55
Closed
AlanRockefeller wants to merge 6 commits intomainfrom
Closed
Conversation
WalkthroughMigrates sidecar metadata keys from filename stems to stable path-based keys, updates call sites to pass Path objects, adds legacy-key migration in SidecarManager, adjusts thumbnail and folder stats metadata lookups, and makes logging directory selection and file-handler setup more robust. Changes
Sequence Diagram(s)sequenceDiagram
participant AC as AppController
participant SM as SidecarManager
participant SJ as Sidecar JSON<br/>entries (faststack.json)
AC->>SM: get_metadata(image_path: Path)
SM->>SM: stable_key = metadata_key_for_path(image_path)
SM->>SJ: entries.get(stable_key)
alt found stable_key
SJ-->>SM: metadata
SM-->>AC: metadata
else not found
SM->>SM: compute candidate keys (stem, ext-preserving, legacy)
SM->>SJ: entries.get(candidate_key)
alt candidate found
SJ-->>SM: legacy_metadata
SM->>SJ: copy/merge to stable_key and delete candidate
SM-->>AC: metadata
else no candidate
SM->>SM: scan entries for matching normalized forms
alt match found
SM->>SJ: rewrite entry under stable_key
SM-->>AC: metadata
else
alt create=True
SM-->>AC: raise ValueError
else
SM-->>AC: None
end
end
end
end
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)
📝 Coding Plan
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: 3eef80314c
ℹ️ 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".
Comment on lines
+164
to
+168
| 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] |
There was a problem hiding this comment.
If a regressed sidecar contains both photo.jpg and photo.CR2, this loop promotes the first candidate it finds to the new photo key and then stops. A get_metadata(Path("photo.jpg")) call therefore leaves photo.CR2 behind, so any flags that only existed on the RAW entry disappear from the visible image and later saves preserve two divergent records instead of fully repairing the filename-key regression.
Useful? React with 👍 / 👎.
Comment on lines
+250
to
+252
| meta = entries.get(filename) | ||
| if meta is None: | ||
| meta = entries.get(stem) |
There was a problem hiding this comment.
On Windows, SidecarManager.metadata_key_for_path() lowercases saved keys via os.path.normcase, but _compute_coverage_buckets() only probes the original-case filename and stem. A common file like IMG_0001.JPG therefore never matches an entry stored as img_0001, so folder coverage sparklines show zero uploaded/stacked/todo coverage even when the sidecar contains those flags.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
faststack/logging_setup.py (1)
27-36: Consider potentialos.access()reliability issues.While the actual file-write test in
_is_writable_diris robust,os.access(parent, os.W_OK)on line 36 can return misleading results on Windows with ACLs or on network filesystems. Since this is a pre-creation heuristic and the actual mkdir will be wrapped in try/except later, this is acceptable, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/logging_setup.py` around lines 27 - 36, The use of os.access(parent, os.W_OK) in _can_create_dir can be unreliable (Windows ACLs, network filesystems); change _can_create_dir to avoid relying on os.access as a pre-check—return parent.is_dir() (i.e., only verify the nearest existing parent is a directory) and rely on the existing try/except around directory creation elsewhere (see _can_create_dir and related _is_writable_dir logic) to catch actual permission errors at mkdir time.faststack/io/sidecar.py (1)
245-259: Consider caching or deferring the filesystem existence check.The
candidate_path.exists()call on line 257 introduces I/O during key normalization. While this enables correct migration of ambiguous legacy keys (stems without extensions), it could cause latency if_stable_key_from_keyis called frequently during the migration scan inget_metadata.If this becomes a performance concern in practice, consider:
- Batch-checking existence upfront during load
- Caching results of existence checks
- Making the existence check opt-in via a parameter
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/io/sidecar.py` around lines 245 - 259, The existence check in _stable_key_from_key (candidate_path.exists()) can cause heavy filesystem I/O during get_metadata migration scans; modify _stable_key_from_key to make this check optional and cheap by adding an opt-in parameter (e.g., check_fs: bool = False) or by consulting a precomputed cache, and update callers (notably get_metadata) to either pass check_fs=True when doing a one-time migration scan or to supply a prepared existence map; keep the existing fallback behavior using metadata_key_for_path and KNOWN_IMAGE_EXTENSIONS, but avoid calling candidate_path.exists() unconditionally—instead read from a passed-in cache or perform batched existence checks up front.faststack/tests/test_sidecar.py (1)
350-361: Consider strengthening the non-Windows assertion.On case-sensitive filesystems (Linux, typical macOS), different casings represent different files. The current assertion only verifies keys are non-empty, but doesn't confirm that
Mixed/Case/IMG_0001.JPGandmixed/case/img_0001.jpgproduce distinct keys.💡 Optional enhancement
if os.name == "nt": assert key_upper == key_lower else: - assert key_upper != "" - assert key_lower != "" + # On case-sensitive filesystems, different casings are distinct files + assert key_upper != key_lower🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/test_sidecar.py` around lines 350 - 361, The test test_metadata_key_for_path_normalizes_case_on_windows currently only asserts non-empty keys on non-Windows systems; change the non-Windows branch to assert that key_upper and key_lower are distinct to ensure differing case yields different stable keys. Locate the test function (test_metadata_key_for_path_normalizes_case_on_windows) and update the else block to assert key_upper != key_lower (using SidecarManager.metadata_key_for_path results), keeping the Windows branch unchanged.
🤖 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 2164-2169: The bulk_map is being keyed with
normalize_path_key(img.path) which differs from the sidecar canonical key used
to look up metadata; change the map to use the sidecar key returned by
sidecar.metadata_key_for_path(img.path) (the local variable key already
computed) so that AppController._get_bulk_metadata_map stores entries under the
same SidecarManager.metadata_key_for_path keys used by consumers like the
thumbnail model and folder stats.
In `@faststack/io/sidecar.py`:
- Around line 186-202: metadata_key_for_path produces forward-slash keys while
normalize_path_key returns OS-native separators on Windows; make them consistent
by normalizing path keys to POSIX-style (forward slashes) in normalize_path_key
(or both) so sidecar lookups and bulk_map use the same key format. Locate
normalize_path_key and change its output to use POSIX separators (e.g., convert
Path or string to use "/" via Path.as_posix() or replace backslashes) and ensure
any callers (e.g., metadata_key_for_path, bulk_map usage) expect the normalized
forward-slash form.
In `@faststack/logging_setup.py`:
- Around line 78-91: When both attempts to create the log directory fail (the
log_dir.mkdir calls), file logging is silently disabled; update the logic around
log_dir/log_file (in logging_setup.py where get_app_data_dir(), log_dir, and
log_file are used) to emit a clear warning to stderr that logs will not be
persisted, and ensure you add the required import (import sys) at the top; keep
the existing fallback to console logging but call something like
sys.stderr.write or logging.warning to notify the user when log_dir becomes None
so the failure is visible.
In `@faststack/thumbnail_view/model.py`:
- Around line 551-553: The metadata lookup uses normalize_path_key while the
metadata map is keyed by the sidecar key; replace uses of
normalize_path_key(img.path) with the sidecar metadata key generator so lookups
match storage — i.e., call
self._get_metadata.sidecar.metadata_key_for_path(img.path) (or the equivalent
SidecarManager.metadata_key_for_path via the _get_metadata.sidecar object) when
indexing metadata_map in the code paths that currently reference
normalize_path_key (e.g., where metadata_map.get(normalize_path_key(img.path),
{}) and the similar lookup later), so keys used for lookup match those produced
by _get_bulk_metadata_map.
---
Nitpick comments:
In `@faststack/io/sidecar.py`:
- Around line 245-259: The existence check in _stable_key_from_key
(candidate_path.exists()) can cause heavy filesystem I/O during get_metadata
migration scans; modify _stable_key_from_key to make this check optional and
cheap by adding an opt-in parameter (e.g., check_fs: bool = False) or by
consulting a precomputed cache, and update callers (notably get_metadata) to
either pass check_fs=True when doing a one-time migration scan or to supply a
prepared existence map; keep the existing fallback behavior using
metadata_key_for_path and KNOWN_IMAGE_EXTENSIONS, but avoid calling
candidate_path.exists() unconditionally—instead read from a passed-in cache or
perform batched existence checks up front.
In `@faststack/logging_setup.py`:
- Around line 27-36: The use of os.access(parent, os.W_OK) in _can_create_dir
can be unreliable (Windows ACLs, network filesystems); change _can_create_dir to
avoid relying on os.access as a pre-check—return parent.is_dir() (i.e., only
verify the nearest existing parent is a directory) and rely on the existing
try/except around directory creation elsewhere (see _can_create_dir and related
_is_writable_dir logic) to catch actual permission errors at mkdir time.
In `@faststack/tests/test_sidecar.py`:
- Around line 350-361: The test
test_metadata_key_for_path_normalizes_case_on_windows currently only asserts
non-empty keys on non-Windows systems; change the non-Windows branch to assert
that key_upper and key_lower are distinct to ensure differing case yields
different stable keys. Locate the test function
(test_metadata_key_for_path_normalizes_case_on_windows) and update the else
block to assert key_upper != key_lower (using
SidecarManager.metadata_key_for_path results), keeping the Windows branch
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7001b833-4984-4c90-946a-adfc631264ff
📒 Files selected for processing (8)
faststack/app.pyfaststack/io/sidecar.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
There was a problem hiding this comment.
Actionable comments posted: 4
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 (1)
354-360:⚠️ Potential issue | 🟠 MajorDon’t capture the initial
SidecarManagerinmetadata_key_fn.Line 360 stores a bound method from the current
self.sidecar, but Line 3560 replacesself.sidecarwhenever the user switches folders. After that, the thumbnail model keeps generating keys with the old manager, so badge/metadata lookups can drift from the active sidecar.🛠️ Suggested fix
- metadata_key_fn=self.sidecar.metadata_key_for_path, + metadata_key_fn=lambda path: self.sidecar.metadata_key_for_path(path),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 354 - 360, The ThumbnailModel is being given a bound method from the current self.sidecar via metadata_key_fn, which captures the initial SidecarManager and breaks when self.sidecar is replaced; change metadata_key_fn to be a lightweight wrapper (e.g., a function or lambda) that looks up self.sidecar at call time and delegates to self.sidecar.metadata_key_for_path so the model always uses the active SidecarManager (refer to ThumbnailModel, metadata_key_fn, and sidecar.metadata_key_for_path).faststack/thumbnail_view/model.py (1)
134-145:⚠️ Potential issue | 🟠 MajorFail fast when
metadata_mapis supplied withoutmetadata_key_fn.
refresh_from_controller(..., metadata_map=...)still accepts a preloaded map, but these branches silently ignore it unless the model was also constructed withmetadata_key_fn. For callers that only providemetadata_map, flag-filtered refreshes can collapse to an empty grid and entry flags fall back toFalseeven though the metadata was already loaded.Suggested guard
- if metadata_map and self._metadata_key_fn: - meta = metadata_map.get(self._metadata_key_fn(img.path), {}) + if metadata_map is not None: + if self._metadata_key_fn is None: + raise ValueError( + "metadata_key_fn is required when metadata_map is provided" + ) + meta = metadata_map.get(self._metadata_key_fn(img.path), {})Apply the same guard in
_add_images_to_entries(...).Also applies to: 552-557, 614-622
🤖 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 134 - 145, Ensure we fail fast when a caller supplies a preloaded metadata_map but the model was not constructed with a metadata_key_fn: in refresh_from_controller (and mirror the same guard in _add_images_to_entries) check if metadata_map is not None and self._metadata_key_fn is None and raise a clear ValueError indicating metadata_key_fn is required when passing metadata_map; update any early-return branches that silently ignore metadata_map so they instead raise, referencing refresh_from_controller, _add_images_to_entries, self._metadata_key_fn and the metadata_map parameter to locate the fixes.
🧹 Nitpick comments (1)
faststack/logging_setup.py (1)
65-69: Logger used beforesetup_logging()completes configuration.
log.warning()is called here, but this function is invoked fromsetup_logging()at line 79 before handlers are attached. Python's "last resort" handler will print to stderr, but the message won't be formatted consistently with the rest of the application logs.Consider using
sys.stderr.write()for consistency with the fallback warning insetup_logging(), or document that this early warning relies on Python's default behavior.♻️ Optional: Use stderr directly for consistency
# Final fallback: system temp is the most reliable writable location. fallback = Path(tempfile.gettempdir()) / "faststack" - log.warning( - "No writable app-data directory found; falling back to temp directory %s. " - "Configuration and logs may not persist across restarts.", - fallback, + sys.stderr.write( + f"WARNING: No writable app-data directory found; falling back to temp " + f"directory {fallback}. Configuration and logs may not persist across restarts.\n" ) return fallback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/logging_setup.py` around lines 65 - 69, The early call to log.warning in the module (the log.warning(...) invocation) runs before setup_logging() attaches handlers, so replace this call with a direct write to stderr (use sys.stderr.write with a trailing newline) to ensure consistent unformatted output, and add the necessary import for sys; alternatively, if you prefer to keep using the logger, move or duplicate the warning so it runs only after setup_logging() completes (i.e., after setup_logging() is called) — reference the log.warning call and the setup_logging() function when making the change.
🤖 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 1047-1048: Replace direct dictionary reads like entries.get(key)
(where key is produced by sidecar.metadata_key_for_path(img.path)) with the
SidecarManager accessor so migrations are applied: call
self.sidecar.get_metadata(img.path, create=False) (or the appropriate
SidecarManager.get_metadata(...) overload) instead of entries.get(key). Update
all occurrences that compute key via sidecar.metadata_key_for_path and then use
entries.get(key) (including the other similar sites referenced) to use
get_metadata(..., create=False) so legacy stem-key entries are resolved and not
dropped.
In `@faststack/io/sidecar.py`:
- Around line 158-168: The current migration logic in sidecar handling
(referencing self.data.entries and the variable names candidate_key, stable_key,
candidate_meta) wrongly moves a bare-stem legacy key (e.g., "photo") under a
folder-specific stable_key (e.g., "a/photo"), stealing metadata; change the
condition so you only migrate a candidate_key when it is unambiguous at root:
skip migration if candidate_key contains no slash (bare stem) but stable_key
contains a slash (folder-specific). Update the same check in the other similar
blocks (the blocks around the other sections that use
candidate_key/candidate_meta and delete candidate_key) so bare-stem keys are
only rewritten when stable_key is also root-level (or otherwise unambiguous).
In `@faststack/logging_setup.py`:
- Around line 28-37: _docstring claims we check writability but _can_create_dir
only verifies existence/is_dir; update the function to also test actual write
permission on the nearest existing parent (e.g., use os.access(parent, os.W_OK)
or equivalent) so it returns True only when parent.is_dir() and writable; add
the necessary import for os if missing and keep the logic in _can_create_dir
(and update any unit tests for get_app_data_dir() if present) so callers won't
get paths under read-only parents.
In `@faststack/tests/test_sidecar.py`:
- Around line 156-178: The test test_legacy_stem_entry_migrates_to_path_key
currently uses a legacy key that equals the stable key returned by
SidecarManager.metadata_key_for_path, so no migration is exercised; change the
seeded content in the mock_sidecar_dir to use a legacy key that differs from
metadata_key_for_path(Path("IMG_0001.jpg")) (for example the stem-only form or
different case) so that calling sm.get_metadata(Path("IMG_0001.jpg"),
create=False) must trigger migration, then assert the migrated entry exists
under sm.metadata_key_for_path(...) and that the original legacy key is removed
from sm.data.entries; reference SidecarManager, metadata_key_for_path,
get_metadata and sm.data.entries when making the changes.
---
Outside diff comments:
In `@faststack/app.py`:
- Around line 354-360: The ThumbnailModel is being given a bound method from the
current self.sidecar via metadata_key_fn, which captures the initial
SidecarManager and breaks when self.sidecar is replaced; change metadata_key_fn
to be a lightweight wrapper (e.g., a function or lambda) that looks up
self.sidecar at call time and delegates to self.sidecar.metadata_key_for_path so
the model always uses the active SidecarManager (refer to ThumbnailModel,
metadata_key_fn, and sidecar.metadata_key_for_path).
In `@faststack/thumbnail_view/model.py`:
- Around line 134-145: Ensure we fail fast when a caller supplies a preloaded
metadata_map but the model was not constructed with a metadata_key_fn: in
refresh_from_controller (and mirror the same guard in _add_images_to_entries)
check if metadata_map is not None and self._metadata_key_fn is None and raise a
clear ValueError indicating metadata_key_fn is required when passing
metadata_map; update any early-return branches that silently ignore metadata_map
so they instead raise, referencing refresh_from_controller,
_add_images_to_entries, self._metadata_key_fn and the metadata_map parameter to
locate the fixes.
---
Nitpick comments:
In `@faststack/logging_setup.py`:
- Around line 65-69: The early call to log.warning in the module (the
log.warning(...) invocation) runs before setup_logging() attaches handlers, so
replace this call with a direct write to stderr (use sys.stderr.write with a
trailing newline) to ensure consistent unformatted output, and add the necessary
import for sys; alternatively, if you prefer to keep using the logger, move or
duplicate the warning so it runs only after setup_logging() completes (i.e.,
after setup_logging() is called) — reference the log.warning call and the
setup_logging() function when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e4cec70-cc33-475c-beaa-079cea71c2a9
📒 Files selected for processing (6)
faststack/app.pyfaststack/io/sidecar.pyfaststack/io/utils.pyfaststack/logging_setup.pyfaststack/tests/test_sidecar.pyfaststack/thumbnail_view/model.py
✅ Files skipped from review due to trivial changes (1)
- faststack/io/utils.py
Owner
Author
|
Closing this one - the changes will be merged in PR 57 |
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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests