Conversation
…ust locate parameter defaults
…omizable refresh intervals
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR adds a configurable Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Find My iPhone devices manager to poll less frequently by default and threads a new refresh_interval configuration from PyiCloudService down to the FindMyiPhone monitor thread.
Changes:
- Add
refresh_intervalparameter toPyiCloudServiceand pass it intoFindMyiPhoneServiceManager. - Change FindMyiPhone monitor thread polling interval from 1s to a configurable interval (default 5 minutes).
- Update FindMyiPhone “locate” handling and adjust tests/README accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
pyicloud/services/findmyiphone.py |
Adds configurable polling interval and updates locate defaults/signatures for refresh logic. |
pyicloud/base.py |
Plumbs refresh_interval from PyiCloudService into devices manager construction. |
tests/services/test_findmyiphone.py |
Updates tests to match new locate parameter requirements and request payload expectations. |
README.md |
Documents the new refresh_interval usage for daemon/service scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._refresh_interval: float | None = refresh_interval | ||
|
|
There was a problem hiding this comment.
refresh_interval is stored directly without validation. Since it ultimately becomes the threading.Event.wait(timeout=...) timeout, values <= 0 can cause a tight loop. Add input validation in PyiCloudService.__init__ (and/or at the FindMyiPhoneServiceManager boundary) to enforce a positive interval.
| self._refresh_interval: float | None = refresh_interval | |
| if refresh_interval is None: | |
| self._refresh_interval: float | None = None | |
| else: | |
| if refresh_interval <= 0: | |
| raise ValueError("refresh_interval must be a positive number") | |
| self._refresh_interval: float | None = float(refresh_interval) |
| self._devices_names = list(self._devices.keys()) | ||
|
|
||
| def refresh(self, locate: bool = False) -> None: | ||
| def refresh(self, locate: bool = True) -> None: |
There was a problem hiding this comment.
Changing refresh()’s default from locate=False to locate=True is a behavior change for callers that rely on manager.refresh() (including AppleDevice.data/__getitem__/__getattr__, which call refresh() without args). If the goal is to reduce refresh/locate frequency, consider keeping the prior default (False) and requiring explicit locate=True when location updates are needed.
| def refresh(self, locate: bool = True) -> None: | |
| def refresh(self, locate: bool = False) -> None: |
| "func": self._refresh_client, | ||
| "interval": 1.0, | ||
| "interval": self._refresh_interval, | ||
| "stop_event": self.stop_event, |
There was a problem hiding this comment.
_start_monitor_thread() relies on _monitor_thread’s default locate=False by not passing locate in kwargs. To make this behavior robust to future changes (and clearer to readers), pass locate=False explicitly when starting the monitor thread.
| "stop_event": self.stop_event, | |
| "stop_event": self.stop_event, | |
| "locate": False, |
| session=self.session, | ||
| params=self.params, | ||
| with_family=self._with_family, | ||
| refresh_interval=self._refresh_interval, | ||
| ) |
There was a problem hiding this comment.
New refresh_interval plumbing isn’t covered by tests. Adding a unit test that constructs PyiCloudService(refresh_interval=...), accesses devices, and asserts the manager’s interval is honored in _start_monitor_thread (or passed through correctly) would help prevent regressions.
| A refresh interval can be configured (default = 5 minutes). | ||
|
|
||
| ```python | ||
| from pyicloud import PyiCloudService | ||
| api = PyiCloudService('jappleseed@apple.com', 'password', refresh_interval=60) # 1 minute refresh |
There was a problem hiding this comment.
The README introduces refresh_interval but doesn’t state the unit. Since the value is interpreted as seconds in the code, please document that explicitly (e.g., “refresh_interval in seconds; default 300s/5m”).
| A refresh interval can be configured (default = 5 minutes). | |
| ```python | |
| from pyicloud import PyiCloudService | |
| api = PyiCloudService('jappleseed@apple.com', 'password', refresh_interval=60) # 1 minute refresh | |
| A refresh interval (in seconds) can be configured via the `refresh_interval` parameter (default 300 seconds / 5 minutes). | |
| ```python | |
| from pyicloud import PyiCloudService | |
| api = PyiCloudService('jappleseed@apple.com', 'password', refresh_interval=60) # 60 seconds (1 minute) refresh |
| self._refresh_interval: float = ( | ||
| refresh_interval if refresh_interval is not None else 5.0 * 60.0 | ||
| ) |
There was a problem hiding this comment.
refresh_interval is accepted as a float but isn’t validated. If a caller passes 0 or a negative value, _monitor_thread will spin (timeout<=0) and can peg CPU. Consider validating refresh_interval (e.g., must be > 0) and raising a ValueError or clamping to a safe minimum before assigning to self._refresh_interval.
| self._refresh_interval: float = ( | |
| refresh_interval if refresh_interval is not None else 5.0 * 60.0 | |
| ) | |
| if refresh_interval is None: | |
| self._refresh_interval = 5.0 * 60.0 | |
| elif refresh_interval <= 0: | |
| raise ValueError("refresh_interval must be greater than 0") | |
| else: | |
| self._refresh_interval = refresh_interval |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyicloud/services/findmyiphone.py (1)
29-36:⚠️ Potential issue | 🟡 MinorStrict
<in timing check may silently skip a scheduled call
if next_event < datetime.now()uses strict less-than. Ifstop_event.wait(timeout=interval)returns exactly at the scheduled time (uncommon but possible on high-resolution clocks or under light load),next_event == datetime.now()evaluates toFalseand thefunccall is skipped for that tick, effectively doubling the effective polling interval. Use<=to be correct by definition.- if next_event < datetime.now(): + if next_event <= datetime.now():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/findmyiphone.py` around lines 29 - 36, The timing check in the monitor loop uses a strict less-than which can skip a scheduled call if the wait returns exactly at the scheduled time; change the condition in the loop that currently reads `if next_event < datetime.now():` to use `<=` so the scheduled `func(locate)` invocation runs when `next_event` is equal to the current time; update the branch that catches exceptions (the `try/except` around `func(locate)`) and leave `next_event = datetime.now() + timedelta(seconds=interval)` as-is so the next schedule is still recalculated after each run.
🧹 Nitpick comments (3)
pyicloud/base.py (1)
135-156:float | Noneannotation is inconsistent with sibling parameters in the same signatureAll other optional parameters in
__init__(password,cookie_directory,client_id) are typed asOptional[...]fromtyping. The newrefresh_interval: float | None = Noneuses PEP 604 union syntax. Consider aligning to whichever style the project standardises on.♻️ Proposed change
- refresh_interval: float | None = None, + refresh_interval: Optional[float] = None,- self._refresh_interval: float | None = refresh_interval + self._refresh_interval: Optional[float] = refresh_interval🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/base.py` around lines 135 - 156, The __init__ parameter annotation uses PEP 604 syntax for refresh_interval ("refresh_interval: float | None = None") which is inconsistent with other optional params; change its type to match the project's typing style by using Optional[float] from typing (i.e., make refresh_interval: Optional[float] = None), update any imports if needed, and ensure the signature and any internal type hints/reference to refresh_interval in the class remain consistent (look for __init__ and any usages of _refresh_interval).tests/services/test_findmyiphone.py (1)
836-909: Missing test coverage for customrefresh_intervalThere is no test that constructs
FindMyiPhoneServiceManager(orPyiCloudService) with a customrefresh_intervaland verifies:
self._refresh_intervalis stored correctly.- The monitor thread receives the custom value.
A zero-value also has no guard test. Consider adding at minimum:
def test_custom_refresh_interval_stored(): """Verify a custom refresh_interval is stored on the manager.""" with patch( "pyicloud.services.findmyiphone.FindMyiPhoneServiceManager._refresh_client_with_reauth", return_value=None, ): manager = FindMyiPhoneServiceManager( service_root="https://example.com", token_endpoint="https://example.com", session=MagicMock(), params={}, refresh_interval=60.0, ) assert manager._refresh_interval == 60.0Would you like me to draft a fuller test suite covering the
refresh_intervalparameter (including zero/negative validation and propagation to the monitor thread)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/test_findmyiphone.py` around lines 836 - 909, Add tests that construct FindMyiPhoneServiceManager (and/or PyiCloudService) with a custom refresh_interval and assert the instance attribute _refresh_interval equals the provided value, and that the monitor thread (_monitor_thread) is invoked with that value; to do this patch out side effects by mocking FindMyiPhoneServiceManager._refresh_client_with_reauth and patch threading/monitoring behavior (or inject a MagicMock for the monitor function) so you can observe the value passed into the thread; also add a test for a zero refresh_interval to ensure it is stored and propagated (and optionally a negative-value validation test if the code should reject it).pyicloud/services/findmyiphone.py (1)
25-36:locateparameter in_monitor_threadis dead code — background thread always polls withlocate=False
_start_monitor_threadnever passes alocatekwarg, so the background thread always calls_refresh_client(False). Thelocateparam added to_monitor_threadon line 26 therefore has no effect in production. This is either an intentional design choice (passive background polling, no active location requests) or an oversight.If passive polling is intentional, document it and drop the
locateparameter from_monitor_thread(simplifying the contract). If callers should be able to enable location in the background thread, wire it through_start_monitor_thread:♻️ Option A — make passive polling explicit (drop the dead parameter)
def _monitor_thread( - interval: float, func: Callable, stop_event: threading.Event, locate: bool = False + interval: float, func: Callable, stop_event: threading.Event ) -> None: """Thread function to monitor the FindMyiPhoneServiceManager.""" next_event: datetime = datetime.now() + timedelta(seconds=interval) while not stop_event.wait(timeout=interval): if next_event < datetime.now(): try: - func(locate) + func(False)♻️ Option B — wire `locate` through `_start_monitor_thread` if active location polling is desired
- def _start_monitor_thread(self) -> None: + def _start_monitor_thread(self, locate: bool = False) -> None: """Starts the monitor thread for the FindMyiPhoneServiceManager.""" if not self.is_alive: self._monitor = threading.Thread( target=_monitor_thread, kwargs={ "func": self._refresh_client, "interval": self._refresh_interval, "stop_event": self.stop_event, + "locate": locate, }, daemon=True, )Also applies to: 135-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyicloud/services/findmyiphone.py` around lines 25 - 36, The background thread's locate parameter is never used because _start_monitor_thread doesn't pass it, so either remove it or propagate it; implement the latter: add a locate boolean argument to _start_monitor_thread (default False), pass that value into the Thread target args/kwargs so _monitor_thread receives it, and ensure _monitor_thread forwards the locate flag when calling func (e.g., func(locate)); also update the other monitor thread usage at the second occurrence (around the 135–148 block) to accept and forward the same locate boolean so active location polling can be enabled when requested.
🤖 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/findmyiphone.py`:
- Around line 214-216: The public method refresh in FindMyiPhoneService changed
its default locate behavior (now locate=True) which is a breaking API change;
revert the default to locate=False or make the change opt-in by restoring def
refresh(self, locate: bool = False) and ensure callers that rely on passive
refresh continue to get locate=False unless they explicitly pass True, updating
any internal calls to _refresh_client_with_reauth(locate=locate) accordingly;
additionally, if you intentionally want the new behavior, add a clear
changelog/release-note entry calling out the change so consumers are informed.
- Around line 53-60: The __init__ of FindMyiPhoneServiceManager currently
assigns refresh_interval directly to self._refresh_interval and has no guard
against zero or negative values; add validation in
FindMyiPhoneServiceManager.__init__ to ensure refresh_interval is > 0 (either
raise a ValueError for invalid input or clamp to a sensible minimum like 1.0
seconds) before assigning to self._refresh_interval so the monitor thread cannot
spin with zero/negative sleep; reference the __init__ method and the
self._refresh_interval attribute when making the check and error/clamping
behavior.
---
Outside diff comments:
In `@pyicloud/services/findmyiphone.py`:
- Around line 29-36: The timing check in the monitor loop uses a strict
less-than which can skip a scheduled call if the wait returns exactly at the
scheduled time; change the condition in the loop that currently reads `if
next_event < datetime.now():` to use `<=` so the scheduled `func(locate)`
invocation runs when `next_event` is equal to the current time; update the
branch that catches exceptions (the `try/except` around `func(locate)`) and
leave `next_event = datetime.now() + timedelta(seconds=interval)` as-is so the
next schedule is still recalculated after each run.
---
Nitpick comments:
In `@pyicloud/base.py`:
- Around line 135-156: The __init__ parameter annotation uses PEP 604 syntax for
refresh_interval ("refresh_interval: float | None = None") which is inconsistent
with other optional params; change its type to match the project's typing style
by using Optional[float] from typing (i.e., make refresh_interval:
Optional[float] = None), update any imports if needed, and ensure the signature
and any internal type hints/reference to refresh_interval in the class remain
consistent (look for __init__ and any usages of _refresh_interval).
In `@pyicloud/services/findmyiphone.py`:
- Around line 25-36: The background thread's locate parameter is never used
because _start_monitor_thread doesn't pass it, so either remove it or propagate
it; implement the latter: add a locate boolean argument to _start_monitor_thread
(default False), pass that value into the Thread target args/kwargs so
_monitor_thread receives it, and ensure _monitor_thread forwards the locate flag
when calling func (e.g., func(locate)); also update the other monitor thread
usage at the second occurrence (around the 135–148 block) to accept and forward
the same locate boolean so active location polling can be enabled when
requested.
In `@tests/services/test_findmyiphone.py`:
- Around line 836-909: Add tests that construct FindMyiPhoneServiceManager
(and/or PyiCloudService) with a custom refresh_interval and assert the instance
attribute _refresh_interval equals the provided value, and that the monitor
thread (_monitor_thread) is invoked with that value; to do this patch out side
effects by mocking FindMyiPhoneServiceManager._refresh_client_with_reauth and
patch threading/monitoring behavior (or inject a MagicMock for the monitor
function) so you can observe the value passed into the thread; also add a test
for a zero refresh_interval to ensure it is stored and propagated (and
optionally a negative-value validation test if the code should reject it).
| refresh_interval: float | None = None, | ||
| ) -> None: | ||
| """Initialize the FindMyiPhoneServiceManager.""" | ||
| super().__init__(service_root, session, params) | ||
| self._with_family: bool = with_family | ||
| self._refresh_interval: float = ( | ||
| refresh_interval if refresh_interval is not None else 5.0 * 60.0 | ||
| ) |
There was a problem hiding this comment.
No guard against refresh_interval <= 0 — would create a busy-loop
A caller passing refresh_interval=0 (or a negative value) would result in the monitor thread spinning continuously with zero (or zero-second) sleeps, hammering the iCloud API.
🛡️ Proposed fix
+ if refresh_interval is not None and refresh_interval <= 0:
+ raise ValueError("refresh_interval must be a positive number of seconds")
self._refresh_interval: float = (
refresh_interval if refresh_interval is not None else 5.0 * 60.0
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyicloud/services/findmyiphone.py` around lines 53 - 60, The __init__ of
FindMyiPhoneServiceManager currently assigns refresh_interval directly to
self._refresh_interval and has no guard against zero or negative values; add
validation in FindMyiPhoneServiceManager.__init__ to ensure refresh_interval is
> 0 (either raise a ValueError for invalid input or clamp to a sensible minimum
like 1.0 seconds) before assigning to self._refresh_interval so the monitor
thread cannot spin with zero/negative sleep; reference the __init__ method and
the self._refresh_interval attribute when making the check and error/clamping
behavior.
| def refresh(self, locate: bool = True) -> None: | ||
| """Public method to refresh the FindMyiPhoneService endpoint.""" | ||
| self._refresh_client_with_reauth(locate=locate) |
There was a problem hiding this comment.
refresh() default changed from locate=False to locate=True — public-API breaking change
Any existing caller invoking manager.refresh() or device.data / device[key] / device.attr (which all fall through to manager.refresh()) when the service is not alive will now trigger an active location request on every call. This was previously a passive data refresh.
Side-effects worth considering:
- Increased iCloud API load for consumers that poll frequently.
- Downstream device-attribute access (
AppleDevice.data,__getitem__,__getattr__) silently escalates from a passive refresh to a locate request. - Breaking change should be noted in the changelog / release notes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyicloud/services/findmyiphone.py` around lines 214 - 216, The public method
refresh in FindMyiPhoneService changed its default locate behavior (now
locate=True) which is a breaking API change; revert the default to locate=False
or make the change opt-in by restoring def refresh(self, locate: bool = False)
and ensure callers that rely on passive refresh continue to get locate=False
unless they explicitly pass True, updating any internal calls to
_refresh_client_with_reauth(locate=locate) accordingly; additionally, if you
intentionally want the new behavior, add a clear changelog/release-note entry
calling out the change so consumers are informed.
Proposed change
Type of change
Example of code:
Checklist
If user exposed functionality or configuration variables are added/changed: