Add CloudKit-backed Photos service and sync support#217
Open
MrJarnould wants to merge 10 commits intotimlaing:mainfrom
Open
Add CloudKit-backed Photos service and sync support#217MrJarnould wants to merge 10 commits intotimlaing:mainfrom
MrJarnould wants to merge 10 commits intotimlaing:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughSplits Photos into CloudKit-backed and legacy modules, adds a typed CloudKit client and Pydantic models, implements a state-backed photo sync engine with SQLite/Memory and materialisation (XMP/EXIF), extends the CLI with photos libraries/get/changes/sync/sync-cursor/watch, and adds many fixtures and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (Typer)"
participant Service as "PhotosService"
participant CK as "CloudKitContainerClient"
participant State as "PhotoSyncState (SQLite/Memory)"
participant FS as "Filesystem"
CLI->>Service: photos sync/watch(options)
Service->>CK: zones_list / database_changes / query / iter_changes
CK-->>Service: typed zone & record responses
Service->>State: open state (create_photo_sync_state)
State-->>Service: previous cursor & resources
Service->>Service: select resources, apply RAW/live-photo rules
Service->>FS: write files atomically, write .xmp, set EXIF
FS-->>Service: write success/failure
Service->>State: upsert_resource / delete_resource / set_sync_cursor
Service-->>CLI: PhotoSyncResult (items, counters, cursor)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.5)pyicloud/services/photos_legacy.pyThanks 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: 10
🧹 Nitpick comments (21)
tests/fixtures/photos_zone_changes_response.json (1)
15-26: Consider the dual deletion representation.The first record contains both an
isDeletedfield (lines 15-19, INT64 value 0) and adeletedproperty (line 26, false). Whilst this dual representation may be intentional for testing field-vs-property precedence or CloudKit's actual response format, ensure it accurately reflects the real API behaviour rather than introducing redundancy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fixtures/photos_zone_changes_response.json` around lines 15 - 26, The fixture shows a record with both an "isDeleted" field and a top-level "deleted" property which may be redundant; update the test fixture so it mirrors real API behavior by removing one representation or aligning their values—either remove the INT64 "isDeleted" field under "fields" or remove the top-level "deleted" property, and ensure any tests that reference "isDeleted" or "deleted" (search for "isDeleted" and "deleted" in test code) are updated to use the single canonical indicator consistently.pyicloud/services/photos_cloudkit/constants.py (1)
107-110: MakePRIMARY_ZONEimmutable to prevent accidental mutation.Using a mutable dict for a module-level constant can lead to subtle runtime side effects.
Suggested refactor
+from types import MappingProxyType + -PRIMARY_ZONE: dict[str, str] = { - "zoneName": "PrimarySync", - "zoneType": "REGULAR_CUSTOM_ZONE", -} +PRIMARY_ZONE = MappingProxyType( + { + "zoneName": "PrimarySync", + "zoneType": "REGULAR_CUSTOM_ZONE", + } +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/constants.py` around lines 107 - 110, PRIMARY_ZONE is currently a mutable module-level dict; replace it with an immutable mapping by importing Mapping and MappingProxyType and assigning PRIMARY_ZONE: Mapping[str, str] = MappingProxyType({"zoneName": "PrimarySync", "zoneType": "REGULAR_CUSTOM_ZONE"}) so callers still get a mapping but attempts to mutate will raise; update the type annotation and add the necessary imports (from typing import Mapping and from types import MappingProxyType) and remove direct dict usage.README.md (1)
940-940: Add missing comma in the upload sentence.Line 940 should read more cleanly as “To upload a photo, use the
uploadmethod.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 940, Update the sentence in README where it currently reads "To upload a photo use the `upload` method" to include the missing comma so it reads "To upload a photo, use the `upload` method." Locate and edit the exact line containing that phrase in the README.md.tests/fixtures/photos_browser_mutations/README.md (1)
8-10: Polish sentence punctuation for readability.Line 9 reads more cleanly with a comma after “instead”.
✏️ Suggested wording tweak
-identifiers. Stable placeholder values are used instead so request and response +identifiers. Stable placeholder values are used instead, so request and response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fixtures/photos_browser_mutations/README.md` around lines 8 - 10, Edit the README sentence that currently reads "Stable placeholder values are used instead so request and response relationships remain testable without exposing personal data." and insert a comma after "instead" so it reads "Stable placeholder values are used instead, so request and response relationships remain testable without exposing personal data." This is the only change needed to improve punctuation and readability in the tests/fixtures/photos_browser_mutations README sentence.pyicloud/cli/normalize.py (1)
160-164: Makezone_nameextraction resilient to typedzone_idobjects.Currently, Line 161-Line 164 only handles dict input; typed model instances will normalise to
zone_name=None.💡 Suggested fix
- if isinstance(zone_id, dict): - zone_name = zone_id.get("zoneName") - else: - zone_name = None + if isinstance(zone_id, dict): + zone_name = zone_id.get("zoneName") + else: + zone_name = ( + getattr(zone_id, "zoneName", None) + or getattr(zone_id, "zone_name", None) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/normalize.py` around lines 160 - 164, The current extraction of zone_name from library.zone_id only handles dicts and falls back to None for typed objects; update the logic around zone_id (from library) to handle dicts and typed model instances by: if zone_id is a dict use zone_id.get("zoneName"), otherwise if zone_id has attributes "zoneName" or "zone_name" use getattr(zone_id, "zoneName", ...) or getattr(zone_id, "zone_name", ...) to pull the value, otherwise set zone_name = None; apply this in the block referencing zone_id and zone_name so typed objects normalize correctly.tests/services/test_photos_sync.py (1)
169-176: Consider extracting repeated cleanup logic into a context manager or fixture.The manual cleanup pattern in
finallyblocks is repeated across all tests. This could be simplified using a context manager,pytestfixture, ortempfile.TemporaryDirectorywhich handles cleanup automatically.♻️ Example using TemporaryDirectory
import tempfile from contextlib import contextmanager `@contextmanager` def sync_test_dir(prefix: str): with tempfile.TemporaryDirectory(prefix=prefix, dir=TEST_BASE) as temp_dir: yield Path(temp_dir) # Usage in test: def test_example(): with sync_test_dir("photos-sync-") as temp_dir: output_dir = temp_dir / "output" state_dir = temp_dir / "state" # ... test logic ...Also applies to: 212-218, 253-259, 282-288, 338-344, 374-380, 430-436, 473-479, 510-516, 541-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/test_photos_sync.py` around lines 169 - 176, Replace the repeated manual cleanup in tests/services/test_photos_sync.py (the finally block that walks temp_dir.rglob("*") and unlinks/rmdirs and then temp_dir.rmdir()) with a reusable context manager or pytest fixture that creates and auto-cleans the temp directory (e.g., a sync_test_dir context manager using tempfile.TemporaryDirectory or a tmp_path fixture); update each test that currently uses the manual finally (references: temp_dir, temp_dir.rglob, and the finally block) to use "with sync_test_dir(... ) as temp_dir:" or the fixture so teardown is automatic and remove the duplicated cleanup code from those tests.pyicloud/services/photos_legacy.py (2)
1734-1744: Response body fully read into memory without size limit.The
download()method reads the entire response body into memory withresponse.raw.read(). For very large photos or videos, this could cause memory pressure. Thestream=Trueparameter is used correctly, but the full body is then loaded at once.This is acceptable for typical photo sizes, but consider documenting or adding optional chunked download support for large video files in future iterations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_legacy.py` around lines 1734 - 1744, The download() method in photos_legacy.py currently calls response.raw.read() which loads the entire response into memory; change it to stream the response in chunks instead: use Response.iter_content(chunk_size=...) (or iterate over response.raw.read into a provided file-like object) and either return a generator/iterator of bytes or accept an optional destination/file handle or max_size parameter to write chunks to disk, preventing unbounded memory use; locate the method named download and the use of self.versions and response.raw.read to implement chunked reads and optional streaming download behavior.
822-852: Pagination termination condition may cause premature exit or missed photos.The pagination loop terminates when fewer than half the page size is returned (
num_results < self.page_size // 2). This heuristic could prematurely exit if the API happens to return a batch that's slightly smaller than expected due to filtering or deduplication, potentially missing legitimate photos. Conversely, the comment suggests this is intentional to handle partial responses.Consider documenting this behaviour more explicitly or using a more robust termination signal (e.g., absence of continuation marker or consistent empty response).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_legacy.py` around lines 822 - 852, The current photos property uses a fragile termination check (num_results < self.page_size // 2) which can skip real photos; update the loop in photos to stop only when the API explicitly signals no more results (use any continuation/has_more marker returned by _get_photos_at if available) or, if the API provides no marker, replace the half-page heuristic with a safer fallback such as breaking only on num_results == 0 or after two consecutive batches smaller than page_size; ensure you update the handling around _get_photos_at, num_results, page_size, photos_ids and offset (used with DirectionEnum) accordingly so offset arithmetic remains correct.pyicloud/services/photos_cloudkit/client.py (2)
143-148: Bareexcept Exceptionmasks specific failure modes.The broad exception handler makes debugging difficult if unexpected errors occur. Consider catching more specific exceptions or at least logging the original exception type for diagnostics.
🔧 Proposed enhancement
try: return data.batch[0].records[0].fields.itemCount.value - except Exception as exc: + except (IndexError, KeyError, AttributeError) as exc: raise CloudKitApiError( "Photos count query failed", payload=data.model_dump(mode="json") ) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/client.py` around lines 143 - 148, The broad "except Exception as exc" around accessing data.batch[0].records[0].fields.itemCount.value should be tightened: catch the likely concrete failures (AttributeError, IndexError, KeyError) instead of Exception, and when raising CloudKitApiError include the original exception type/message in the raised error (e.g., add exc.__class__.__name__ and str(exc) into the error message or payload) so diagnostics show the underlying cause while preserving the existing data.model_dump(mode="json") payload; keep the "from exc" chaining to retain the traceback.
108-148: Accessing private_httpattribute couples implementation to internal client details.The
batch_countmethod accessesself._client._http.build_url(...), which relies on a private attribute. This creates tight coupling to the internal structure ofCloudKitContainerClient. Consider exposing abuild_urlmethod on the public client interface or delegating the entire batch-count operation to the underlying client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/client.py` around lines 108 - 148, The batch_count method currently reaches into a private attribute (self._client._http.build_url) which couples it to CloudKitContainerClient internals; change batch_count to use a public API instead (e.g., call a new or existing self._client.build_url(...) or delegate the whole batch-count call to a new public method on CloudKitContainerClient such as request_batch_count or build_url that encapsulates URL construction and HTTP details). Update batch_count to call that public method (or delegate the request) and remove direct use of _http; ensure you reference the PhotosBatchCountRequest/PhotosBatchCountResponse modeling and preserve error handling (CloudKitApiError) while passing the payload and response through the public client method.pyicloud/cli/commands/photos.py (1)
955-961: Simplify redundant condition.The condition
iterations and iterations > 1is redundant. Sinceiterationshasmin=1validation (line 869), wheniterationsis truthy it's always>= 1. The check could be simplified.Suggested fix
if only_print_filenames: - if iterations is None or (iterations and iterations > 1): + if iterations is None or iterations > 1: state.console.print(f"run {payload['iteration']}") for item in payload["items"]: state.console.print(item["path"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/cli/commands/photos.py` around lines 955 - 961, The conditional "if iterations is None or (iterations and iterations > 1)" is overly verbose; simplify it in the photos command where only_print_filenames is handled by replacing the redundant "(iterations and iterations > 1)" with "iterations > 1" so the check becomes "if iterations is None or iterations > 1" (use the existing variables iterations, only_print_filenames, payload and state.console.print to locate the block and preserve the logic that prints "run {payload['iteration']}" when iterations is unset or greater than 1).pyicloud/services/photos_cloudkit/models.py (1)
160-163: PreferTYPE_CHECKINGoverif Falsefor type-only imports.Using
if Falseworks but is unconventional. The standard pattern usestyping.TYPE_CHECKINGwhich is more explicit and self-documenting.Suggested fix
+from typing import TYPE_CHECKING + # Import-only type hints to avoid circular imports at runtime. -if False: # pragma: no cover +if TYPE_CHECKING: from .constants import DirectionEnum, ListTypeEnum, ObjectTypeEnum from .service import BasePhotoAlbum, PhotoAsset🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/models.py` around lines 160 - 163, Replace the nonstandard "if False: # pragma: no cover" block used for import-only type hints with the standard typing.TYPE_CHECKING guard: import TYPE_CHECKING from typing and change the conditional to "if TYPE_CHECKING:" and keep the same names (DirectionEnum, ListTypeEnum, ObjectTypeEnum, BasePhotoAlbum, PhotoAsset) inside that block so type-only imports are clear and avoid runtime circular imports.pyicloud/services/photos_cloudkit/sync.py (1)
622-632: Unbounded loop in_unique_relative_pathcould theoretically run indefinitely.While extremely unlikely in practice, the
while Trueloop has no upper bound. If the path collision logic has a bug, this could hang. Consider adding a reasonable iteration limit.Suggested defensive limit
discriminator = asset_id[:8] index = 1 - while True: + max_attempts = 10000 + while index <= max_attempts: suffix_bits = f"_{discriminator}" if index == 1 else f"_{discriminator}_{index}" next_path = (directory / f"{stem}{suffix_bits}{suffix}").as_posix() owner = tracked_paths.get(next_path) if next_path not in reserved_paths and ( owner is None or owner == (asset_id, resource_key) ): return next_path index += 1 + raise PhotosServiceException( + f"Could not find unique path for {candidate} after {max_attempts} attempts" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/sync.py` around lines 622 - 632, The _unique_relative_path function uses an unbounded while True loop with variables discriminator and index to find a non-colliding next_path against reserved_paths and tracked_paths; add a defensive maximum iteration limit (e.g., MAX_ATTEMPTS constant) and increment index each loop, and if the limit is reached raise a clear exception (e.g., RuntimeError or custom) including asset_id and resource_key so callers can fail fast and debugging information is present; update any callers or tests as needed to handle the new exception.tests/services/test_photos_cloudkit_client.py (1)
180-192: Consider using a consistent mocking approach across tests.Lines 185, 203, and 226 directly patch
client._client._http.post, reaching two levels into private implementation details. This coupling makes tests brittle if the internal structure changes. The earlier tests (lines 36-56, 62-76) mocksession.postat the session level which is more resilient.If the typed client path requires different mocking, consider extracting a helper or documenting why the different approach is necessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/test_photos_cloudkit_client.py` around lines 180 - 192, The test is directly mocking deep internals (client._client._http.post) which couples tests to implementation; instead construct PhotosCloudKitClient with a MagicMock session and mock session.post (the same pattern used earlier), e.g., pass a MagicMock as the session argument and set session.post.return_value to ZONES_LIST_PAYLOAD before calling client.zones_list(), or extract a test helper (e.g., make_mocked_photos_client(session_response)) that builds a PhotosCloudKitClient and configures the session.post mock; update the assertions to use that session-level mock and remove references to _client._http.post in the test.pyicloud/common/cloudkit/client.py (2)
211-237: In-place mutation of request for pagination could be made more explicit.The
iter_changesmethod mutatesreq.zones[0].syncTokenin place (line 237) to advance pagination. While functional, this pattern can be surprising. A brief comment would improve clarity.Suggested documentation
if not zone.moreComing: return + # Advance the sync token for the next page req.zones[0].syncToken = zone.syncToken🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/common/cloudkit/client.py` around lines 211 - 237, The iter_changes function mutates req.zones[0].syncToken in-place to advance pagination which is surprising; update the code to make this explicit by either (A) adding a short clarifying comment immediately above the line that sets req.zones[0].syncToken explaining that the request object is intentionally reused to carry the next syncToken for subsequent pages, or (B) avoid in-place mutation by creating a new CKZoneChangesRequest (copying existing req, updating the zones[0].syncToken) and assigning it back to req before the next loop iteration; reference the iter_changes function and the req.zones[0].syncToken update when making the change.
89-91: Clarify the intent of defaulting non-integer status codes to 200.The fallback
code = 200whenstatus_codeis not an integer appears designed to handle mocked responses, but it could inadvertently mask issues where a mock returns an unexpected type. Consider either:
- Raising an error for invalid response types
- Adding a comment explaining this is for mock compatibility
Also applies to: 122-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/common/cloudkit/client.py` around lines 89 - 91, The current fallback of setting code = 200 when resp.status_code is not an int masks unexpected/mock errors; change this to explicitly validate and raise a TypeError (or ValueError) instead of silently defaulting: check getattr(resp, "status_code", None), if it's not an int raise TypeError("Invalid response.status_code: ...") referencing resp and its status_code value, and apply the same change to the other identical block (the code/status_code handling around lines 122-124) so tests that intentionally mock must provide an int while real failures surface immediately.pyicloud/services/photos_cloudkit/state.py (2)
173-197: Consider batching commits for bulk sync operations.Each
upsert_resourcecall commits immediately (line 197). During large sync runs with many resources, this could become a performance bottleneck. Consider either:
- Accepting an optional
commit=Trueparameter- Providing a batch upsert method
- Using a context manager pattern for transaction batching
This is a minor concern for typical usage but may matter for large libraries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/state.py` around lines 173 - 197, The upsert_resource method commits every call which hurts performance for bulk syncs; modify upsert_resource (and its use sites) to accept an optional commit: bool = True parameter and only call self.conn.commit() when commit is True (leaving callers able to defer commits), and add either a new batch_upsert_resources(resources: Iterable[SyncedPhotoResource]) that executes many INSERT ... ON CONFLICT statements inside a single transaction (begin/commit around the loop) or provide a context manager on the state class that begins a transaction on __enter__ and commits on __exit__ so callers can group multiple upserts without committing each time; update callers to use the new parameter or batch/context API.
85-86:PRAGMA foreign_keys=ONhas no effect without foreign key constraints.The schema does not define any foreign key constraints, so this pragma is a no-op. Either remove it or add the intended foreign key relationships.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/state.py` around lines 85 - 86, The PRAGMA enabling foreign keys is a no-op because no tables define FOREIGN KEY constraints; either remove the line self._conn.execute("PRAGMA foreign_keys=ON") from the connection setup or actually define the intended foreign key relationships in the schema/table creation code so the pragma has effect (update the table CREATE statements to include FOREIGN KEY clauses referencing the related tables). Locate the connection initialization where self._conn.execute("PRAGMA journal_mode=WAL") and self._conn.execute("PRAGMA foreign_keys=ON") are called and choose one of the two fixes: remove the foreign_keys pragma if no FK constraints are planned, or add explicit FOREIGN KEY definitions to the relevant CREATE TABLE statements so the pragma is meaningful.pyicloud/services/photos_cloudkit/service.py (3)
320-325: Mock-specific branching in production constructor.The
if _is_mock_like(service)check hardcodes test behaviour into the production__init__. This special-casing makes the class harder to understand and maintain.Consider either:
- Using a factory function that returns different implementations for testing
- Passing an explicit
skip_indexing_checkparameter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/service.py` around lines 320 - 325, The constructor contains test-specific branching via _is_mock_like(service) and type(self).__name__ != "PhotoLibrary"; remove this implicit mock detection and instead either (a) add an explicit constructor parameter like skip_indexing_check: bool = False (or skip_indexing: bool = False) to the class __init__ and use that to set self._indexing_state = "FINISHED" and return when True, or (b) create a factory function (e.g., make_photo_library_for_test / PhotoLibraryTestStub factory) that returns a test-specific implementation that sets _indexing_state without changing production __init__; update callers/tests to pass the new flag or use the factory and delete the _is_mock_like(...) branching and the type(self).__name__ check from the production constructor.
1118-1125: Early exit condition may under-fetch with small page sizes.The condition
num_results < self.page_size // 2could cause premature exit whenpage_sizeis small (e.g., ifpage_sizeis 1, thenpage_size // 2is 0, and any result would exit). Givenpage_sizeis capped at 100 (line 926), this is unlikely to be an issue, but the logic is subtle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/service.py` around lines 1118 - 1125, The loop's early-exit check using `num_results < self.page_size // 2` can prematurely stop fetching when `page_size` is small; change the condition in the loop that follows `seen.add(photo.id)`/`yield photo` to require either no results or apply a minimum threshold, e.g. `if num_results == 0: break` or `if num_results < max(1, self.page_size // 2): break`, so that `_direction`/`offset` adjustments and pagination continue correctly for small `page_size` values; update the conditional referencing `num_results`, `self.page_size`, and keep `self._direction`/`DirectionEnum` logic unchanged.
11-11: Production code depends onunittest.mock.Mockfor runtime behaviour.Importing
Mockfromunittest.mockin production code and using_is_mock_like()checks throughout creates tight coupling between production and test code. This pattern:
- Adds test framework as a production dependency
- Makes the code harder to reason about
- Could mask bugs if mock detection fails
Consider using dependency injection or a dedicated test double interface instead.
Also applies to: 215-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/service.py` at line 11, The code imports unittest.mock.Mock and uses _is_mock_like() runtime checks, coupling production to test framework; replace direct dependence on Mock by removing the import and change _is_mock_like to use an injectable predicate or duck-typed check instead: add an optional parameter (e.g., is_mock_like: Callable[..., bool]) to the PhotosCloudKitService constructor or to the functions that call _is_mock_like, defaulting to a conservative runtime check (e.g., check for specific attributes/methods used by the code) so tests can pass a mock-detection helper without importing unittest.mock in production; update all usages of _is_mock_like (including the occurrences around the 215-224 region) to call the injected predicate and remove any direct isinstance(..., Mock) checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples.py`:
- Around line 450-451: The disposable album naming uses
datetime.utcnow().strftime with second-level precision (album_name) which can
collide on rapid retries; update album_name (and consequently renamed_name) to
include higher precision or a random suffix—e.g., append microseconds via "%f"
in the strftime pattern or add a short uuid (uuid.uuid4().hex[:8])—so
functions/variables album_name and renamed_name become collision-safe in
parallel or fast re-run scenarios.
In `@pyicloud/services/photos_cloudkit/models.py`:
- Around line 145-150: Fix the docstring typo in the PhotosUploadError model:
update the class PhotosUploadError's docstring to replace "uploadimagews" with a
clear phrase such as "upload images" (e.g., "One upload images error item.") so
the docstring reads correctly and describes the error item for upload images.
In `@pyicloud/services/photos_cloudkit/sync.py`:
- Around line 704-707: The code assumes asset.asset_date exists and is not None;
change to safely fetch and validate it: use asset_date = getattr(asset,
"asset_date", None) and if asset_date is None (or not a datetime-like object)
return False before calling .astimezone(); then compute age_days = (now_local -
asset_date.astimezone()).days and compare to options.keep_icloud_recent_days.
This prevents AttributeError and handles None asset_date gracefully (refer to
asset, asset_date, now_local, and options.keep_icloud_recent_days).
- Around line 260-262: The comparison can raise TypeError when asset.added_date
is None; in the loop over assets from _iter_sync_assets, guard before comparing:
retrieve added_date = getattr(asset, "added_date", None) and skip the asset
(continue) or treat missing as older/newer according to intent if added_date is
None, otherwise compare added_date < cutoff; update the loop in sync.py to use
this explicit None-check around the comparison to avoid None < cutoff errors.
In `@pyicloud/services/photos_legacy.py`:
- Around line 341-358: The code reads
response["records"][0]["fields"]["state"]["value"] without verifying the
CloudKit response shape, which can raise IndexError/KeyError; update the block
after request.json() to defensively validate that response is a dict with a
non-empty "records" list and that the first record contains
"fields"->"state"->"value" (or use chained .get checks or a try/except around
the extraction), and if validation fails log a debug/error and raise
PyiCloudServiceNotActivatedException (or the same error path used when
indexing_state != "FINISHED") so unexpected shapes are handled gracefully;
reference the variables response, indexing_state, self.service.session.post,
zone_id and the PyiCloudServiceNotActivatedException when making the change.
- Around line 1746-1774: The delete() method currently returns success based
only on resp.status_code == 200; update it to parse and validate the JSON
payload returned by self._service.session.post (call resp.json()) and ensure the
CloudKit response contains no error entries (e.g., check top-level "results" and
each result's "status"/"error" fields or absence of "error") before returning
True; if the JSON indicates failure, return False (or surface/log the error) so
delete() only returns True when status code is 200 and the response body
confirms a successful operation.
In `@README.md`:
- Around line 123-131: Remove the leading shell prompt markers ("$ ") from the
example command lines so they are plain code examples (e.g., the lines starting
with "$ icloud photos libraries", "$ icloud photos list", "$ icloud photos
sync", etc.), which will satisfy markdownlint MD014; update each example in the
README command block to be the command only (no "$ " prefix) or alternatively
wrap the entire set in a fenced code block without prompt markers.
- Around line 813-818: Update the example CLI invocations so the shared library
placeholder is quoted to prevent shell interpreting <zoneName> as redirection;
specifically modify the example commands for "icloud photos list", "icloud
photos get", and "icloud photos sync" that use the shared:<zoneName> placeholder
(e.g., the lines showing icloud photos list --library shared:<zoneName>, icloud
photos get ... --library shared:<zoneName>, and icloud photos sync --library
shared:<zoneName>) to wrap the entire shared:<zoneName> value in single or
double quotes (e.g., 'shared:<zoneName>') in all occurrences mentioned.
In `@tests/fixtures/photos_browser_mutations/README.md`:
- Around line 4-6: The README contains an absolute filesystem path
"/Users/jacob/Documents/GitHub/pyicloud/tests/fixtures/README.md" which won't
work elsewhere; update the markdown link in
tests/fixtures/photos_browser_mutations/README.md to use a repository-relative
path (e.g., change the link target to tests/fixtures/README.md or
./../README.md) so the reference resolves correctly for other developers and CI.
In `@tests/fixtures/README.md`:
- Around line 22-24: Replace the machine-specific absolute link in the README
entry that references photos_browser_mutations with a repository-relative link;
locate the line that mentions `photos_browser_mutations` in
tests/fixtures/README.md and change the URL from the local absolute path to a
relative path like `tests/fixtures/photos_browser_mutations/README.md` so the
link works in-repo and on GitHub.
---
Nitpick comments:
In `@pyicloud/cli/commands/photos.py`:
- Around line 955-961: The conditional "if iterations is None or (iterations and
iterations > 1)" is overly verbose; simplify it in the photos command where
only_print_filenames is handled by replacing the redundant "(iterations and
iterations > 1)" with "iterations > 1" so the check becomes "if iterations is
None or iterations > 1" (use the existing variables iterations,
only_print_filenames, payload and state.console.print to locate the block and
preserve the logic that prints "run {payload['iteration']}" when iterations is
unset or greater than 1).
In `@pyicloud/cli/normalize.py`:
- Around line 160-164: The current extraction of zone_name from library.zone_id
only handles dicts and falls back to None for typed objects; update the logic
around zone_id (from library) to handle dicts and typed model instances by: if
zone_id is a dict use zone_id.get("zoneName"), otherwise if zone_id has
attributes "zoneName" or "zone_name" use getattr(zone_id, "zoneName", ...) or
getattr(zone_id, "zone_name", ...) to pull the value, otherwise set zone_name =
None; apply this in the block referencing zone_id and zone_name so typed objects
normalize correctly.
In `@pyicloud/common/cloudkit/client.py`:
- Around line 211-237: The iter_changes function mutates req.zones[0].syncToken
in-place to advance pagination which is surprising; update the code to make this
explicit by either (A) adding a short clarifying comment immediately above the
line that sets req.zones[0].syncToken explaining that the request object is
intentionally reused to carry the next syncToken for subsequent pages, or (B)
avoid in-place mutation by creating a new CKZoneChangesRequest (copying existing
req, updating the zones[0].syncToken) and assigning it back to req before the
next loop iteration; reference the iter_changes function and the
req.zones[0].syncToken update when making the change.
- Around line 89-91: The current fallback of setting code = 200 when
resp.status_code is not an int masks unexpected/mock errors; change this to
explicitly validate and raise a TypeError (or ValueError) instead of silently
defaulting: check getattr(resp, "status_code", None), if it's not an int raise
TypeError("Invalid response.status_code: ...") referencing resp and its
status_code value, and apply the same change to the other identical block (the
code/status_code handling around lines 122-124) so tests that intentionally mock
must provide an int while real failures surface immediately.
In `@pyicloud/services/photos_cloudkit/client.py`:
- Around line 143-148: The broad "except Exception as exc" around accessing
data.batch[0].records[0].fields.itemCount.value should be tightened: catch the
likely concrete failures (AttributeError, IndexError, KeyError) instead of
Exception, and when raising CloudKitApiError include the original exception
type/message in the raised error (e.g., add exc.__class__.__name__ and str(exc)
into the error message or payload) so diagnostics show the underlying cause
while preserving the existing data.model_dump(mode="json") payload; keep the
"from exc" chaining to retain the traceback.
- Around line 108-148: The batch_count method currently reaches into a private
attribute (self._client._http.build_url) which couples it to
CloudKitContainerClient internals; change batch_count to use a public API
instead (e.g., call a new or existing self._client.build_url(...) or delegate
the whole batch-count call to a new public method on CloudKitContainerClient
such as request_batch_count or build_url that encapsulates URL construction and
HTTP details). Update batch_count to call that public method (or delegate the
request) and remove direct use of _http; ensure you reference the
PhotosBatchCountRequest/PhotosBatchCountResponse modeling and preserve error
handling (CloudKitApiError) while passing the payload and response through the
public client method.
In `@pyicloud/services/photos_cloudkit/constants.py`:
- Around line 107-110: PRIMARY_ZONE is currently a mutable module-level dict;
replace it with an immutable mapping by importing Mapping and MappingProxyType
and assigning PRIMARY_ZONE: Mapping[str, str] = MappingProxyType({"zoneName":
"PrimarySync", "zoneType": "REGULAR_CUSTOM_ZONE"}) so callers still get a
mapping but attempts to mutate will raise; update the type annotation and add
the necessary imports (from typing import Mapping and from types import
MappingProxyType) and remove direct dict usage.
In `@pyicloud/services/photos_cloudkit/models.py`:
- Around line 160-163: Replace the nonstandard "if False: # pragma: no cover"
block used for import-only type hints with the standard typing.TYPE_CHECKING
guard: import TYPE_CHECKING from typing and change the conditional to "if
TYPE_CHECKING:" and keep the same names (DirectionEnum, ListTypeEnum,
ObjectTypeEnum, BasePhotoAlbum, PhotoAsset) inside that block so type-only
imports are clear and avoid runtime circular imports.
In `@pyicloud/services/photos_cloudkit/service.py`:
- Around line 320-325: The constructor contains test-specific branching via
_is_mock_like(service) and type(self).__name__ != "PhotoLibrary"; remove this
implicit mock detection and instead either (a) add an explicit constructor
parameter like skip_indexing_check: bool = False (or skip_indexing: bool =
False) to the class __init__ and use that to set self._indexing_state =
"FINISHED" and return when True, or (b) create a factory function (e.g.,
make_photo_library_for_test / PhotoLibraryTestStub factory) that returns a
test-specific implementation that sets _indexing_state without changing
production __init__; update callers/tests to pass the new flag or use the
factory and delete the _is_mock_like(...) branching and the type(self).__name__
check from the production constructor.
- Around line 1118-1125: The loop's early-exit check using `num_results <
self.page_size // 2` can prematurely stop fetching when `page_size` is small;
change the condition in the loop that follows `seen.add(photo.id)`/`yield photo`
to require either no results or apply a minimum threshold, e.g. `if num_results
== 0: break` or `if num_results < max(1, self.page_size // 2): break`, so that
`_direction`/`offset` adjustments and pagination continue correctly for small
`page_size` values; update the conditional referencing `num_results`,
`self.page_size`, and keep `self._direction`/`DirectionEnum` logic unchanged.
- Line 11: The code imports unittest.mock.Mock and uses _is_mock_like() runtime
checks, coupling production to test framework; replace direct dependence on Mock
by removing the import and change _is_mock_like to use an injectable predicate
or duck-typed check instead: add an optional parameter (e.g., is_mock_like:
Callable[..., bool]) to the PhotosCloudKitService constructor or to the
functions that call _is_mock_like, defaulting to a conservative runtime check
(e.g., check for specific attributes/methods used by the code) so tests can pass
a mock-detection helper without importing unittest.mock in production; update
all usages of _is_mock_like (including the occurrences around the 215-224
region) to call the injected predicate and remove any direct isinstance(...,
Mock) checks.
In `@pyicloud/services/photos_cloudkit/state.py`:
- Around line 173-197: The upsert_resource method commits every call which hurts
performance for bulk syncs; modify upsert_resource (and its use sites) to accept
an optional commit: bool = True parameter and only call self.conn.commit() when
commit is True (leaving callers able to defer commits), and add either a new
batch_upsert_resources(resources: Iterable[SyncedPhotoResource]) that executes
many INSERT ... ON CONFLICT statements inside a single transaction (begin/commit
around the loop) or provide a context manager on the state class that begins a
transaction on __enter__ and commits on __exit__ so callers can group multiple
upserts without committing each time; update callers to use the new parameter or
batch/context API.
- Around line 85-86: The PRAGMA enabling foreign keys is a no-op because no
tables define FOREIGN KEY constraints; either remove the line
self._conn.execute("PRAGMA foreign_keys=ON") from the connection setup or
actually define the intended foreign key relationships in the schema/table
creation code so the pragma has effect (update the table CREATE statements to
include FOREIGN KEY clauses referencing the related tables). Locate the
connection initialization where self._conn.execute("PRAGMA journal_mode=WAL")
and self._conn.execute("PRAGMA foreign_keys=ON") are called and choose one of
the two fixes: remove the foreign_keys pragma if no FK constraints are planned,
or add explicit FOREIGN KEY definitions to the relevant CREATE TABLE statements
so the pragma is meaningful.
In `@pyicloud/services/photos_cloudkit/sync.py`:
- Around line 622-632: The _unique_relative_path function uses an unbounded
while True loop with variables discriminator and index to find a non-colliding
next_path against reserved_paths and tracked_paths; add a defensive maximum
iteration limit (e.g., MAX_ATTEMPTS constant) and increment index each loop, and
if the limit is reached raise a clear exception (e.g., RuntimeError or custom)
including asset_id and resource_key so callers can fail fast and debugging
information is present; update any callers or tests as needed to handle the new
exception.
In `@pyicloud/services/photos_legacy.py`:
- Around line 1734-1744: The download() method in photos_legacy.py currently
calls response.raw.read() which loads the entire response into memory; change it
to stream the response in chunks instead: use
Response.iter_content(chunk_size=...) (or iterate over response.raw.read into a
provided file-like object) and either return a generator/iterator of bytes or
accept an optional destination/file handle or max_size parameter to write chunks
to disk, preventing unbounded memory use; locate the method named download and
the use of self.versions and response.raw.read to implement chunked reads and
optional streaming download behavior.
- Around line 822-852: The current photos property uses a fragile termination
check (num_results < self.page_size // 2) which can skip real photos; update the
loop in photos to stop only when the API explicitly signals no more results (use
any continuation/has_more marker returned by _get_photos_at if available) or, if
the API provides no marker, replace the half-page heuristic with a safer
fallback such as breaking only on num_results == 0 or after two consecutive
batches smaller than page_size; ensure you update the handling around
_get_photos_at, num_results, page_size, photos_ids and offset (used with
DirectionEnum) accordingly so offset arithmetic remains correct.
In `@README.md`:
- Line 940: Update the sentence in README where it currently reads "To upload a
photo use the `upload` method" to include the missing comma so it reads "To
upload a photo, use the `upload` method." Locate and edit the exact line
containing that phrase in the README.md.
In `@tests/fixtures/photos_browser_mutations/README.md`:
- Around line 8-10: Edit the README sentence that currently reads "Stable
placeholder values are used instead so request and response relationships remain
testable without exposing personal data." and insert a comma after "instead" so
it reads "Stable placeholder values are used instead, so request and response
relationships remain testable without exposing personal data." This is the only
change needed to improve punctuation and readability in the
tests/fixtures/photos_browser_mutations README sentence.
In `@tests/fixtures/photos_zone_changes_response.json`:
- Around line 15-26: The fixture shows a record with both an "isDeleted" field
and a top-level "deleted" property which may be redundant; update the test
fixture so it mirrors real API behavior by removing one representation or
aligning their values—either remove the INT64 "isDeleted" field under "fields"
or remove the top-level "deleted" property, and ensure any tests that reference
"isDeleted" or "deleted" (search for "isDeleted" and "deleted" in test code) are
updated to use the single canonical indicator consistently.
In `@tests/services/test_photos_cloudkit_client.py`:
- Around line 180-192: The test is directly mocking deep internals
(client._client._http.post) which couples tests to implementation; instead
construct PhotosCloudKitClient with a MagicMock session and mock session.post
(the same pattern used earlier), e.g., pass a MagicMock as the session argument
and set session.post.return_value to ZONES_LIST_PAYLOAD before calling
client.zones_list(), or extract a test helper (e.g.,
make_mocked_photos_client(session_response)) that builds a PhotosCloudKitClient
and configures the session.post mock; update the assertions to use that
session-level mock and remove references to _client._http.post in the test.
In `@tests/services/test_photos_sync.py`:
- Around line 169-176: Replace the repeated manual cleanup in
tests/services/test_photos_sync.py (the finally block that walks
temp_dir.rglob("*") and unlinks/rmdirs and then temp_dir.rmdir()) with a
reusable context manager or pytest fixture that creates and auto-cleans the temp
directory (e.g., a sync_test_dir context manager using
tempfile.TemporaryDirectory or a tmp_path fixture); update each test that
currently uses the manual finally (references: temp_dir, temp_dir.rglob, and the
finally block) to use "with sync_test_dir(... ) as temp_dir:" or the fixture so
teardown is automatic and remove the duplicated cleanup code from those tests.
🪄 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: 9de70243-1d50-4d20-a924-5cdf99572f25
📒 Files selected for processing (66)
README.mdexamples.pypyicloud/cli/commands/photos.pypyicloud/cli/normalize.pypyicloud/common/cloudkit/__init__.pypyicloud/common/cloudkit/client.pypyicloud/common/cloudkit/models.pypyicloud/services/photos.pypyicloud/services/photos_cloudkit/__init__.pypyicloud/services/photos_cloudkit/client.pypyicloud/services/photos_cloudkit/constants.pypyicloud/services/photos_cloudkit/mappers.pypyicloud/services/photos_cloudkit/materialize.pypyicloud/services/photos_cloudkit/models.pypyicloud/services/photos_cloudkit/queries.pypyicloud/services/photos_cloudkit/service.pypyicloud/services/photos_cloudkit/state.pypyicloud/services/photos_cloudkit/sync.pypyicloud/services/photos_legacy.pytests/fixtures/README.mdtests/fixtures/photos_album_create_response.jsontests/fixtures/photos_album_membership_query_core.jsontests/fixtures/photos_album_membership_response.jsontests/fixtures/photos_album_rename_response.jsontests/fixtures/photos_all_photos_query_core.jsontests/fixtures/photos_all_photos_response.jsontests/fixtures/photos_browser_mutations/README.mdtests/fixtures/photos_browser_mutations/album_add_photo_request.jsontests/fixtures/photos_browser_mutations/album_add_photo_response.jsontests/fixtures/photos_browser_mutations/album_create_request.jsontests/fixtures/photos_browser_mutations/album_create_response.jsontests/fixtures/photos_browser_mutations/album_delete_request.jsontests/fixtures/photos_browser_mutations/album_delete_response.jsontests/fixtures/photos_browser_mutations/album_remove_photo_request.jsontests/fixtures/photos_browser_mutations/album_remove_photo_response.jsontests/fixtures/photos_browser_mutations/album_rename_request.jsontests/fixtures/photos_browser_mutations/album_rename_response.jsontests/fixtures/photos_browser_mutations/photo_delete_request.jsontests/fixtures/photos_browser_mutations/photo_delete_response.jsontests/fixtures/photos_database_changes_response.jsontests/fixtures/photos_favorites_query_core.jsontests/fixtures/photos_favorites_response.jsontests/fixtures/photos_indexing_not_finished_response.jsontests/fixtures/photos_live_photo_response.jsontests/fixtures/photos_missing_counterparts_response.jsontests/fixtures/photos_recently_added_query_core.jsontests/fixtures/photos_recently_added_response.jsontests/fixtures/photos_shared_library_all_photos_query_core.jsontests/fixtures/photos_shared_library_all_photos_response.jsontests/fixtures/photos_shared_library_favorites_query_core.jsontests/fixtures/photos_shared_library_favorites_response.jsontests/fixtures/photos_shared_library_private_zones_response.jsontests/fixtures/photos_shared_library_shared_zones_response.jsontests/fixtures/photos_shared_library_unfavorite_request.jsontests/fixtures/photos_shared_library_unfavorite_response.jsontests/fixtures/photos_shared_library_zone_changes_request.jsontests/fixtures/photos_shared_library_zone_changes_response.jsontests/fixtures/photos_upload_duplicate_response.jsontests/fixtures/photos_upload_skeletal_response.jsontests/fixtures/photos_video_only_response.jsontests/fixtures/photos_zone_changes_response.jsontests/fixtures/photos_zones_list_response.jsontests/services/test_photos.pytests/services/test_photos_cloudkit_client.pytests/services/test_photos_sync.pytests/test_cmdline.py
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
README.md (1)
772-775: Prefer fully-qualified command names for consistency.Line 772 and Line 774 mix full commands (e.g.,
icloud photos libraries) with shorthand entries (e.g.,albums,list). Using the full form for each item would reduce ambiguity for quick copy/paste reading.Suggested wording tweak
-- `icloud photos libraries`, `albums`, `list`, `get`, `changes`, and `sync-cursor` +- `icloud photos libraries`, `icloud photos albums`, `icloud photos list`, + `icloud photos get`, `icloud photos changes`, and `icloud photos sync-cursor` are read-focused inspection commands. -- `icloud photos sync` and `icloud photos watch` are the modern replacement path +- `icloud photos sync` and `icloud photos watch` are the modern replacement path for `icloud_photos_downloader`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 772 - 775, Update the README to use fully-qualified command names for consistency: replace shorthand entries `albums`, `list`, `get`, `changes`, and `sync-cursor` with their full forms prefixed by the command namespace (e.g., `icloud photos albums`, `icloud photos list`, `icloud photos get`, `icloud photos changes`, `icloud photos sync-cursor`), and ensure `icloud photos sync` and `icloud photos watch` remain as-is while referencing `icloud_photos_downloader` only where appropriate.pyicloud/services/photos_cloudkit/sync.py (3)
627-635: Unbounded loop with no explicit limit.The
while Trueloop relies on eventually finding a unique path. Whilst practically safe (the 8-character discriminator plus incrementing index provides ample namespace), consider adding an upper bound as a defensive measure against pathological cases:Optional defensive limit
index = 1 - while True: + while index < 10000: suffix_bits = f"_{discriminator}" if index == 1 else f"_{discriminator}_{index}" next_path = (directory / f"{stem}{suffix_bits}{suffix}").as_posix() owner = tracked_paths.get(next_path) if next_path not in reserved_paths and ( owner is None or owner == (asset_id, resource_key) ): return next_path index += 1 + raise PhotosServiceException(f"Could not generate unique path for {candidate}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/sync.py` around lines 627 - 635, The infinite retry loop that builds candidate paths (using discriminator, index, reserved_paths, tracked_paths, asset_id, resource_key and next_path) needs a defensive upper bound to avoid pathological hangs; replace the while True with a bounded loop (e.g. max_attempts constant) and increment index each attempt, returning next_path if valid and after max_attempts raise a clear exception (or propagate a specific error) indicating no unique path could be found so callers can handle the failure.
375-394: Consider wrapping remote delete in error handling.If
asset.delete()raises an exception (e.g., network error, API failure), the sync will abort without completing the remaining assets or persisting partial progress. This could leave the sync in an inconsistent state where some assets were processed but the cursor isn't updated.Consider catching and logging delete failures to allow the sync to continue:
Suggested approach
if _should_delete_remote_asset( asset=asset, options=options, asset_ready_for_delete=asset_ready_for_delete, asset_confirmed_local=asset_confirmed_local, now_local=now_local, ): - deleted = asset.delete() - sync_complete = False - if deleted: + try: + deleted = asset.delete() + except Exception: + deleted = False + if deleted: + sync_complete = False result.items.append(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/sync.py` around lines 375 - 394, The remote deletion call asset.delete() inside the block guarded by _should_delete_remote_asset(...) should be wrapped in try/except so a thrown exception doesn't abort the whole sync; catch exceptions around the asset.delete() call in the same scope, log the exception (including asset.id and exception text) and append a PhotoSyncItem indicating the delete failure (e.g., resource_key="remote", action="delete-failed", reason containing the error or "delete-error") to result.items and increment an appropriate counter (or leave result.deleted_count unchanged) while ensuring sync_complete remains False so the loop continues processing other assets; keep the existing successful-delete handling for when delete() returns truthy.
511-516: Defensive guard forphotosattribute.
getattr(album, "photos")without a default will raiseAttributeErrorif the attribute is missing, orTypeErrorif it returnsNone. Consider adding a guard or default:Suggested fix
- for asset in getattr(album, "photos"): + photos = getattr(album, "photos", None) + if photos is None: + continue + for asset in photos:The same pattern appears at line 528 for
source.photos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_cloudkit/sync.py` around lines 511 - 516, The loop over getattr(album, "photos") can raise if "photos" is missing or None; change the guard to use a safe default or check before iterating (e.g., use getattr(album, "photos", []) or if getattr(album, "photos") is not None and iterable) so the for asset in ... loop in this function will skip when photos is absent, and apply the same defensive change for the similar loop using source.photos at the other occurrence; keep the existing seen set and yield asset behavior 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 `@pyicloud/services/photos_cloudkit/sync.py`:
- Around line 406-422: The loop over tracked_resources currently calls
stale_path.unlink() without handling exceptions which can abort processing and
leave state inconsistent; wrap the unlink call in a try/except that catches
OSError/PermissionError, log the failure (using the module or existing logger),
but regardless of unlink success proceed to call
state.delete_resource(stale.asset_id, stale.resource_key), append the
PhotoSyncItem (marking action "deleted" and optionally include a note or flag if
the local delete failed), and increment result.deleted_count so remaining stale
entries are processed; reference the loop handling tracked_resources, the
stale_path.unlink() call, state.delete_resource(...), result.items append, and
result.deleted_count to locate where to add the try/except and logging.
In `@pyicloud/services/photos_legacy.py`:
- Around line 560-568: The upload code assumes self._upload_url always exists,
causing requests like "None/upload?..." for discovered non-root libraries; fix
by either propagating the upload URL when creating zone libraries (update the
libraries() codepath to pass the parent's upload URL into
PhotoLibrary/PhotoAlbum constructors or PhotoLibrary.__init__) or by failing
fast in upload paths (add a guard in PhotoAlbum.upload() and upload_file() to
check self._upload_url and raise a clear exception if missing). Update all
upload callsites (upload_file, PhotoAlbum.upload and any library construction in
libraries()) to use the chosen approach so no upload proceeds when _upload_url
is None.
- Around line 661-665: Currently _root_library is eagerly constructed with
PhotoLibrary(self, PRIMARY_ZONE, upload_url=upload_url) which triggers indexing
in PhotoLibrary.__init__ and can raise PyiCloudServiceNotActivatedException for
shared-only callers; change _root_library to be created lazily: store None
initially and only instantiate PhotoLibrary(self, PRIMARY_ZONE,
upload_url=upload_url) inside the root-specific accessor methods (the properties
or functions that access _root_library), leaving _shared_library untouched so
shared-only paths never hit PhotoLibrary constructor until root access is
requested. Ensure any code that reads _root_library checks for None and
constructs it atomically (or via a helper like _get_root_library()) to avoid
races.
- Around line 1441-1446: The try/except in photos_legacy.py that sets
self.creation_date currently only catches ValueError; expand the exception
handler to also catch TypeError and OverflowError so non-numeric/None or extreme
creation_date values don't abort shared-stream listing. Update the except on the
block that calls int(creation_date) / datetime.fromtimestamp in the
PhotosLegacy/album record handling (the self.creation_date assignment) to catch
(ValueError, TypeError, OverflowError) and keep the existing fallback to epoch.
- Around line 1116-1125: The code currently unconditionally copies
payload["records"][0]["recordChangeTag"] and modification date into
self._record_change_tag/_record_modification_date even when the response is a
CPLContainerRelation; change it to only refresh the cached album metadata when
the returned record is actually the album (e.g., check
payload["records"][0].get("recordType") == "CPLAlbum" or
payload["records"][0].get("recordName") indicates an album) and otherwise leave
self._record_change_tag and self._record_modification_date untouched so
subsequent rename() or delete() use the correct album change tag.
- Around line 917-922: After converting a negative index (key) to its positive
form, validate that the adjusted key is within bounds (0 <= key < len(self)) and
raise IndexError if it's out of range (also handle len(self) == 0); do this
check before calling _get_photos_at so that values below -len(self) no longer
get clamped to 0 and return the first photo. In other words, in the block that
adjusts negative key and then calls _get_photos_at, add a bounds check on the
adjusted key and raise IndexError when invalid instead of relying on
_get_photos_at to handle it.
---
Nitpick comments:
In `@pyicloud/services/photos_cloudkit/sync.py`:
- Around line 627-635: The infinite retry loop that builds candidate paths
(using discriminator, index, reserved_paths, tracked_paths, asset_id,
resource_key and next_path) needs a defensive upper bound to avoid pathological
hangs; replace the while True with a bounded loop (e.g. max_attempts constant)
and increment index each attempt, returning next_path if valid and after
max_attempts raise a clear exception (or propagate a specific error) indicating
no unique path could be found so callers can handle the failure.
- Around line 375-394: The remote deletion call asset.delete() inside the block
guarded by _should_delete_remote_asset(...) should be wrapped in try/except so a
thrown exception doesn't abort the whole sync; catch exceptions around the
asset.delete() call in the same scope, log the exception (including asset.id and
exception text) and append a PhotoSyncItem indicating the delete failure (e.g.,
resource_key="remote", action="delete-failed", reason containing the error or
"delete-error") to result.items and increment an appropriate counter (or leave
result.deleted_count unchanged) while ensuring sync_complete remains False so
the loop continues processing other assets; keep the existing successful-delete
handling for when delete() returns truthy.
- Around line 511-516: The loop over getattr(album, "photos") can raise if
"photos" is missing or None; change the guard to use a safe default or check
before iterating (e.g., use getattr(album, "photos", []) or if getattr(album,
"photos") is not None and iterable) so the for asset in ... loop in this
function will skip when photos is absent, and apply the same defensive change
for the similar loop using source.photos at the other occurrence; keep the
existing seen set and yield asset behavior unchanged.
In `@README.md`:
- Around line 772-775: Update the README to use fully-qualified command names
for consistency: replace shorthand entries `albums`, `list`, `get`, `changes`,
and `sync-cursor` with their full forms prefixed by the command namespace (e.g.,
`icloud photos albums`, `icloud photos list`, `icloud photos get`, `icloud
photos changes`, `icloud photos sync-cursor`), and ensure `icloud photos sync`
and `icloud photos watch` remain as-is while referencing
`icloud_photos_downloader` only where appropriate.
🪄 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: 5f674115-7506-4311-9d45-ccc28f947be0
📒 Files selected for processing (9)
README.mdexamples.pypyicloud/services/photos_cloudkit/models.pypyicloud/services/photos_cloudkit/sync.pypyicloud/services/photos_legacy.pytests/fixtures/README.mdtests/fixtures/photos_browser_mutations/README.mdtests/services/test_photos.pytests/services/test_photos_sync.py
✅ Files skipped from review due to trivial changes (3)
- tests/fixtures/README.md
- examples.py
- pyicloud/services/photos_cloudkit/models.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/services/test_photos_sync.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pyicloud/services/photos_legacy.py (1)
694-715: Consider validating thezoneskey before iteration.Line 700 directly accesses
response["zones"]without checking if the key exists. If the CloudKit API returns an unexpected response shape, this will raise an unhandledKeyError.🛡️ Proposed defensive access
response: dict[str, Any] = request.json() - zones: list[dict[str, Any]] = response["zones"] + zones: list[dict[str, Any]] = response.get("zones", []) libraries: dict[str, BasePhotoLibrary] = { "root": self._get_root_library(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_legacy.py` around lines 694 - 715, The code assumes response["zones"] exists and will KeyError on unexpected shapes; change the access in the method that builds libraries so you defensively obtain zones (e.g., zones = response.get("zones") and if it's None or not a list handle it by using an empty list or logging/raising a clear error) before iterating to create PhotoLibrary instances; ensure you still populate libraries with self._get_root_library() and self._shared_library and assign self._libraries accordingly when zones are missing or invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyicloud/services/photos_legacy.py`:
- Around line 1657-1663: The added_date property (in photos_legacy.py) directly
indexes self._asset_record["fields"]["addedDate"]["value"] which can raise
KeyError if the field is missing; update the added_date getter to mirror the
defensive pattern used by asset_date: wrap access to
self._asset_record["fields"]["addedDate"]["value"] in a try/except (catch
KeyError/TypeError/ValueError as needed) and return datetime.fromtimestamp(0,
timezone.utc) as a fallback (epoch) when the value is absent or invalid,
ensuring the property always returns a datetime instead of raising.
- Around line 1079-1101: In delete() validate the response payload before using
payload["records"][0]: ensure payload is a dict, payload.get("records") is a
list and len(...) > 0; only then update self._record_change_tag and
self._record_modification_date from records[0]; otherwise log a warning or error
and leave the existing self._record_change_tag / self._record_modification_date
unchanged (same defensive pattern used in rename() and add_photo()), and raise
or return a PhotosServiceException if you want to treat a missing/empty records
array as an error.
---
Nitpick comments:
In `@pyicloud/services/photos_legacy.py`:
- Around line 694-715: The code assumes response["zones"] exists and will
KeyError on unexpected shapes; change the access in the method that builds
libraries so you defensively obtain zones (e.g., zones = response.get("zones")
and if it's None or not a list handle it by using an empty list or
logging/raising a clear error) before iterating to create PhotoLibrary
instances; ensure you still populate libraries with self._get_root_library() and
self._shared_library and assign self._libraries accordingly when zones are
missing or invalid.
🪄 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: 024a5f37-a104-44fd-90cd-fee5abff4734
📒 Files selected for processing (4)
pyicloud/services/photos_cloudkit/sync.pypyicloud/services/photos_legacy.pytests/services/test_photos.pytests/services/test_photos_sync.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/services/test_photos_sync.py
- pyicloud/services/photos_cloudkit/sync.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pyicloud/services/photos_legacy.py (1)
1515-1523: Avoid forcing album-size lookup during shared-stream paging.Line 1521 calls
len(self)to caplimit, which triggerswebgetassetcountbefore paging. That conflicts with the_get_photo()intent to avoid an upfront size lookup and adds latency.Refactor to remove the eager size dependency
def _get_payload( self, offset: int, page_size: int, direction: DirectionEnum ) -> dict[str, Any]: return { "albumguid": self._album_guid, "albumctag": self._album_ctag, - "limit": str(min(offset + page_size, len(self))), + "limit": str(offset + page_size), "offset": str(offset), }Also applies to: 1533-1549
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/photos_legacy.py` around lines 1515 - 1523, The payload builder _get_payload currently calls len(self) (triggering webgetassetcount) to cap "limit", which forces an upfront album-size lookup; change it to avoid any size-dependent call and set "limit" to the requested page_size (e.g., "limit": str(page_size)) so paging uses the page size only and does not call len(self); apply the same change to the other payload/construction site referenced in the review (the similar block around lines 1533-1549) and ensure consumers relying on the old capped value still work with page_size-only limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyicloud/services/photos_legacy.py`:
- Around line 1037-1054: The rename/delete/add_photo handlers currently assume a
successful POST on no exception; instead inspect the parsed response payload for
error indications before mutating state or returning success: in the methods
(e.g., rename, delete, add_photo) check payload for keys like "errors" or
absence of a successful "records" entry (and validate that records[0] contains
expected fields) and only then update _record_change_tag,
_record_modification_date, and _name or return True; if the payload indicates
failure, do not change those instance attributes and return False (or raise an
appropriate exception) so callers don't receive a false success.
- Around line 199-211: parse_asset_response currently indexes nested keys
directly and can raise KeyError on malformed records; update the function
(parse_asset_response) to defensively validate each rec from response["records"]
before using nested fields: ensure rec is a dict, check rec.get("recordType")
and only access rec.get("fields") when present and is a dict, and verify
masterRef/value/recordName exist and are strings before assigning into
asset_records; similarly validate CPLMaster entries before appending to
master_records; skip (and optionally log) any malformed recs so one bad record
doesn't abort the entire parse.
---
Nitpick comments:
In `@pyicloud/services/photos_legacy.py`:
- Around line 1515-1523: The payload builder _get_payload currently calls
len(self) (triggering webgetassetcount) to cap "limit", which forces an upfront
album-size lookup; change it to avoid any size-dependent call and set "limit" to
the requested page_size (e.g., "limit": str(page_size)) so paging uses the page
size only and does not call len(self); apply the same change to the other
payload/construction site referenced in the review (the similar block around
lines 1533-1549) and ensure consumers relying on the old capped value still work
with page_size-only limits.
🪄 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: 1139b362-b18e-49ac-b3f3-5c73c7fbb401
📒 Files selected for processing (2)
pyicloud/services/photos_legacy.pytests/services/test_photos.py
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.
Breaking change
No breaking change is intended for supported public usage.
This PR adds a CloudKit-backed Photos implementation for private and Shared Library content while preserving the legacy Shared Albums stream behavior behind the
sharedlibrary key. CloudKit-only commands now explicitly reject the legacysharedkey and requirerootor a Shared Library key likeshared:<zoneName>, which makes the service split explicit and avoids ambiguous or incorrect behavior.Proposed change
This PR modernizes the Photos service by introducing a CloudKit-backed implementation for private and Shared Library photo access while keeping the legacy Shared Albums path available separately.
It adds support for library discovery and library-scoped access to
rootand Shared Library zones, and expands the Photos CLI with CloudKit-backedlibraries,list,get,download,changes,sync-cursor,sync, andwatchflows.It also adds sync features and follow-up fixes validated during smoke testing, including:
LibraryandFavoritesauto-deletekeep-icloud-recent-days--recentType of change
Example of code:
Additional information
UV_CACHE_DIR=/tmp/uv-cache UV_NO_EDITABLE=1 uv run --no-sync pytest tests/services/test_photos.py tests/services/test_photos_cloudkit_client.py tests/services/test_photos_sync.py tests/test_cmdline.py -q222 passed--align-rawcoverage.Checklist
If user exposed functionality or configuration variables are added/changed: