Merge pull request #94 from samspade21/claude/review-code-bugs-5BMmj

Refactor: Extract constants and add request rate limiting

Author: samspade21

custom_components/vacasa/api_client.py

@@ -17,12 +17,14 @@
 from .const import (
     API_BASE_TEMPLATE,
     AUTH_URL,
+    CLIENT_ID_CACHE_TTL,
     DEFAULT_API_VERSION,
     DEFAULT_CACHE_TTL,
     DEFAULT_CLIENT_ID,
     DEFAULT_CONN_TIMEOUT,
     DEFAULT_JITTER_MAX,
     DEFAULT_KEEPALIVE_TIMEOUT,
+    DEFAULT_MAX_CONCURRENT_REQUESTS,
     DEFAULT_MAX_CONNECTIONS,
     DEFAULT_READ_TIMEOUT,
     DEFAULT_TIMEOUT,
@@ -143,6 +145,9 @@ def __init__(
             hass=hass,
         )
 
+        # Semaphore to cap concurrent API requests and avoid rate-limit errors
+        self._request_semaphore = asyncio.Semaphore(DEFAULT_MAX_CONCURRENT_REQUESTS)
+
         # Set up retry handler
         self._retry_handler = RetryWithBackoff(
             max_retries=max_retries,
@@ -224,7 +229,8 @@ async def _ensure_client_id(self) -> str:
         """Ensure the OAuth client ID is up to date."""
         now = time.time()
         cache_valid = (
-            self._client_id_last_fetch is not None and now - self._client_id_last_fetch < 3600
+            self._client_id_last_fetch is not None
+            and now - self._client_id_last_fetch < CLIENT_ID_CACHE_TTL
         )
 
         if cache_valid and self._client_id:
@@ -335,77 +341,80 @@ async def _request(
         for version in self._version_candidates(version_override):
             url = self._build_api_url(path, version)
             try:
-                async with session.request(
-                    method,
-                    url,
-                    params=params,
-                    json=json_data,
-                    headers=self._get_headers(),
-                    timeout=DEFAULT_TIMEOUT,
-                ) as response:
-                    if response.status in acceptable_status:
-                        self._set_api_version(version)
-                        if not return_json:
-                            return await response.text()
-                        # Always attempt JSON parsing when return_json=True
-                        # API may include charset in content-type
-                        # (e.g., "application/json; charset=utf-8")
-                        try:
-                            return await response.json()
-                        except (aiohttp.ContentTypeError, json.JSONDecodeError) as e:
-                            # Log diagnostic info for troubleshooting
-                            response_text = await response.text()
+                async with self._request_semaphore:
+                    async with session.request(
+                        method,
+                        url,
+                        params=params,
+                        json=json_data,
+                        headers=self._get_headers(),
+                        timeout=DEFAULT_TIMEOUT,
+                    ) as response:
+                        if response.status in acceptable_status:
+                            self._set_api_version(version)
+                            if not return_json:
+                                return await response.text()
+                            # Always attempt JSON parsing when return_json=True
+                            # API may include charset in content-type
+                            # (e.g., "application/json; charset=utf-8")
+                            try:
+                                return await response.json()
+                            except (aiohttp.ContentTypeError, json.JSONDecodeError) as e:
+                                # Log diagnostic info for troubleshooting
+                                response_text = await response.text()
+                                _LOGGER.warning(
+                                    "Failed to parse JSON from %s (content-type: %s): %s. Response: %s",
+                                    url,
+                                    response.content_type,
+                                    e,
+                                    response_text[:200],
+                                )
+                                raise ApiError(
+                                    f"Non-JSON response from {url}: {response_text[:200]}"
+                                ) from e
+
+                        if response.status == 401:
+                            # Attempt token refresh once when unauthorized
                             _LOGGER.warning(
-                                "Failed to parse JSON from %s (content-type: %s): %s. Response: %s",
-                                url,
-                                response.content_type,
-                                e,
-                                response_text[:200],
+                                "API request unauthorized for %s, refreshing token", url
                             )
-                            # Return text as fallback, but this will likely
-                            # cause errors in calling code
-                            return response_text
-
-                    if response.status == 401:
-                        # Attempt token refresh once when unauthorized
-                        _LOGGER.warning("API request unauthorized for %s, refreshing token", url)
-                        if retry_on_unauthorized:
-                            await self.authenticate()
-                            await self._save_token_to_cache()
-                            return await self._request(
-                                method,
+                            if retry_on_unauthorized:
+                                await self.authenticate()
+                                await self._save_token_to_cache()
+                                return await self._request(
+                                    method,
+                                    path,
+                                    params=params,
+                                    json_data=json_data,
+                                    acceptable_status=acceptable_status,
+                                    version_override=version,
+                                    return_json=return_json,
+                                    retry_on_unauthorized=False,
+                                )
+                            last_error = AuthenticationError("Unauthorized")
+                            continue
+
+                        if response.status == 403:
+                            last_error = AuthenticationError(
+                                f"Forbidden request to {path}: {response.status}"
+                            )
+                            break
+
+                        # Try next version on not found errors when fallbacks are allowed
+                        if response.status in (404, 400) and version_override is None:
+                            _LOGGER.debug(
+                                "API version %s returned %s for %s, trying fallback",
+                                version,
+                                response.status,
                                 path,
-                                params=params,
-                                json_data=json_data,
-                                acceptable_status=acceptable_status,
-                                version_override=version,
-                                return_json=return_json,
-                                retry_on_unauthorized=False,
                             )
-                        last_error = AuthenticationError("Unauthorized")
-                        continue
-
-                    if response.status == 403:
-                        last_error = AuthenticationError(
-                            f"Forbidden request to {path}: {response.status}"
-                        )
-                        break
+                            last_error = ApiError(f"Endpoint {path} unavailable")
+                            continue
 
-                    # Try next version on not found errors when fallbacks are allowed
-                    if response.status in (404, 400) and version_override is None:
-                        _LOGGER.debug(
-                            "API version %s returned %s for %s, trying fallback",
-                            version,
-                            response.status,
-                            path,
+                        response_text = await response.text()
+                        last_error = ApiError(
+                            f"Unexpected status {response.status} for {path}: {response_text[:200]}"
                         )
-                        last_error = ApiError(f"Endpoint {path} unavailable")
-                        continue
-
-                    response_text = await response.text()
-                    last_error = ApiError(
-                        f"Unexpected status {response.status} for {path}: {response_text[:200]}"
-                    )
             except AuthenticationError:
                 raise
             except aiohttp.ClientError as err:
@@ -866,11 +875,15 @@ async def _follow_auth_redirects(self, initial_url: str) -> str | None:
                                 return token
 
                         # If we've reached owners.vacasa.com without a token, try one more request
-                        # Use proper hostname parsing to prevent URL manipulation attacks
+                        # Use domain-parts check: ensure last 3 labels are owners.vacasa.com
+                        # This prevents false matches like fake-owners.vacasa.com
                         response_host = str(response.url.host).lower() if response.url.host else ""
-                        if response_host == "owners.vacasa.com" or response_host.endswith(
-                            ".owners.vacasa.com"
-                        ):
+                        host_parts = response_host.split(".")
+                        is_owners_host = (
+                            len(host_parts) >= 3
+                            and ".".join(host_parts[-3:]) == "owners.vacasa.com"
+                        )
+                        if is_owners_host:
                             page_content = await response.text()
                             token_match = re.search(r'access_token=([^&"\']+)', page_content)
                             if token_match:

custom_components/vacasa/binary_sensor.py

@@ -2,7 +2,6 @@
 
 from __future__ import annotations
 
-import asyncio
 import logging
 from datetime import datetime
 from typing import Any
@@ -176,20 +175,14 @@ def _refresh_from_coordinator(self) -> None:
             return
         self._update_from_state(state)
 
+    @callback
     def _handle_reservation_state(self, unit_id: str, state: ReservationState) -> None:
         """Handle reservation updates sent by the calendar entities."""
         if unit_id != self._unit_id:
             return
 
         self._update_from_state(state)
-        # Schedule state write on event loop thread-safely
-        # Signal handlers execute on worker threads, so we must use call_soon_threadsafe
-        hass = getattr(self, "hass", None)
-        loop = getattr(hass, "loop", None)
-        if isinstance(loop, asyncio.AbstractEventLoop):
-            loop.call_soon_threadsafe(self.async_write_ha_state)
-        else:
-            self.async_write_ha_state()
+        self.async_write_ha_state()
 
     def _update_from_state(self, state: ReservationState) -> None:
         """Store reservation state and mark availability."""
@@ -201,8 +194,8 @@ def _update_from_state(self, state: ReservationState) -> None:
 
         # Log occupancy changes to help diagnose timing issues
         if old_occupancy != new_occupancy:
-            _LOGGER.warning(
-                "OCCUPANCY CHANGED for %s: %s -> %s. Current reservation: %s",
+            _LOGGER.info(
+                "Occupancy changed for %s: %s -> %s. Current reservation: %s",
                 self._name,
                 "occupied" if old_occupancy else "vacant",
                 "occupied" if new_occupancy else "vacant",

custom_components/vacasa/calendar.py

@@ -16,6 +16,10 @@
 from . import VacasaConfigEntry, VacasaDataUpdateCoordinator
 from .api_client import VacasaApiClient
 from .const import (
+    CALENDAR_LOOKAHEAD_DAYS,
+    CALENDAR_LOOKBACK_DAYS,
+    DEFAULT_CHECKIN_TIME,
+    DEFAULT_CHECKOUT_TIME,
     DOMAIN,
     SIGNAL_RESERVATION_BOUNDARY,
     SIGNAL_RESERVATION_STATE,
@@ -252,10 +256,10 @@ async def _determine_current_and_next_events(
         """Determine the current and next events for the property."""
         now_local = dt_util.now()
         now_utc = dt_util.utcnow()
-        # Query from 60 days ago to catch any currently active reservations that started in the past
-        # but are still in progress (e.g., guest checking out today but checked in weeks ago)
-        start_date = now_local - timedelta(days=60)
-        end_date = now_local + timedelta(days=365)
+        # Query from CALENDAR_LOOKBACK_DAYS ago to catch active reservations started in the past
+        # (e.g., guest checking out today but checked in weeks ago)
+        start_date = now_local - timedelta(days=CALENDAR_LOOKBACK_DAYS)
+        end_date = now_local + timedelta(days=CALENDAR_LOOKAHEAD_DAYS)
         events = await self.async_get_events(self.hass, start_date, end_date)
 
         _LOGGER.debug(
@@ -370,7 +374,7 @@ def _schedule_boundary_timers(self) -> None:
                     partial(self._handle_boundary_timer, boundary="checkout"),
                     end_utc,
                 )
-                _LOGGER.warning(
+                _LOGGER.debug(
                     "Scheduled checkout refresh for %s at %s "
                     "(local: %s, original: %s with tz: %s). Event: %s",
                     self._name,
@@ -389,7 +393,7 @@ def _schedule_boundary_timers(self) -> None:
                     partial(self._handle_boundary_timer, boundary="checkin"),
                     start_utc,
                 )
-                _LOGGER.warning(
+                _LOGGER.debug(
                     "Scheduled check-in refresh for %s at %s "
                     "(local: %s, original: %s with tz: %s). Event: %s",
                     self._name,
@@ -409,8 +413,8 @@ def _handle_boundary_timer(self, scheduled_time: datetime, *, boundary: str) ->
         if not self.hass:
             return
 
-        _LOGGER.warning(
-            "BOUNDARY TIMER (%s) FIRED for %s! Scheduled: %s, Actual: %s",
+        _LOGGER.debug(
+            "Boundary timer (%s) fired for %s. Scheduled: %s, Actual: %s",
             boundary,
             self._name,
             scheduled_time.isoformat(),
@@ -532,8 +536,8 @@ def _reservation_to_event(  # noqa: C901
                 # Use property-specific time
                 start_dt_str = f"{start_date}T{property_checkin_time}"
             else:
-                # No time available, default to 4:00 PM (16:00)
-                start_dt_str = f"{start_date}T16:00:00"
+                # No time available, use default check-in time
+                start_dt_str = f"{start_date}T{DEFAULT_CHECKIN_TIME}"
 
             if checkout_time:
                 # Use time from reservation
@@ -542,8 +546,8 @@ def _reservation_to_event(  # noqa: C901
                 # Use property-specific time
                 end_dt_str = f"{end_date}T{property_checkout_time}"
             else:
-                # No time available, default to 10:00 AM (10:00)
-                end_dt_str = f"{end_date}T10:00:00"
+                # No time available, use default checkout time
+                end_dt_str = f"{end_date}T{DEFAULT_CHECKOUT_TIME}"
 
             # Parse datetime strings
             start_dt = dt_util.parse_datetime(start_dt_str)
@@ -638,14 +642,14 @@ def _reservation_to_event(  # noqa: C901
             elif self._checkin_time:
                 description_parts.append(f"Check-in: {start_date} {self._checkin_time[:5]}")
             else:
-                description_parts.append(f"Check-in: {start_date} 16:00")
+                description_parts.append(f"Check-in: {start_date} {DEFAULT_CHECKIN_TIME[:5]}")
 
             if checkout_time and checkout_time != "00:00:00":
                 description_parts.append(f"Check-out: {end_date} {checkout_time[:5]}")
             elif self._checkout_time:
                 description_parts.append(f"Check-out: {end_date} {self._checkout_time[:5]}")
             else:
-                description_parts.append(f"Check-out: {end_date} 10:00")
+                description_parts.append(f"Check-out: {end_date} {DEFAULT_CHECKOUT_TIME[:5]}")
 
             description_parts.append("")
 

custom_components/vacasa/const.py

@@ -20,6 +20,7 @@
 # Performance optimization settings
 DEFAULT_CACHE_TTL = 3600  # seconds (1 hour) for property data
 DEFAULT_MAX_CONNECTIONS = 10
+DEFAULT_MAX_CONCURRENT_REQUESTS = 5  # max simultaneous API requests
 DEFAULT_KEEPALIVE_TIMEOUT = 30  # seconds
 DEFAULT_CONN_TIMEOUT = 30  # seconds
 DEFAULT_READ_TIMEOUT = 30  # seconds
@@ -90,3 +91,14 @@
 # Dispatcher signals
 SIGNAL_RESERVATION_BOUNDARY = "vacasa_reservation_boundary"
 SIGNAL_RESERVATION_STATE = "vacasa_reservation_state"
+
+# Calendar event window constants
+CALENDAR_LOOKBACK_DAYS = 60  # days to look back for active reservations
+CALENDAR_LOOKAHEAD_DAYS = 365  # days to look ahead for future reservations
+
+# Default reservation times when none are provided by the API
+DEFAULT_CHECKIN_TIME = "16:00:00"  # 4:00 PM
+DEFAULT_CHECKOUT_TIME = "10:00:00"  # 10:00 AM
+
+# Client ID cache TTL (re-use DEFAULT_CACHE_TTL for this purpose)
+CLIENT_ID_CACHE_TTL = DEFAULT_CACHE_TTL  # seconds