Skip to content

Conversation

@CoMPaTech
Copy link
Owner

@CoMPaTech CoMPaTech commented Aug 16, 2025

Summary by CodeRabbit

  • New Features

    • Added firmware update workflow: check (with optional force), download, progress, and install.
    • Added device warnings retrieval and provisioning-mode toggle.
  • Documentation

    • Expanded README with new API calls and example payloads for Update, Progress, Install, and Warnings.
    • Added CHANGELOG entry for 0.4.0 (2025-08-16).
  • Chores

    • Bumped package version to 0.4.0a2 and updated branding/description to "Ubiquiti".
  • Tests

    • Unified tests to use a single HTTP request path and adjusted mocks and response handling.

@coderabbitai
Copy link

coderabbitai bot commented Aug 16, 2025

Warning

Rate limit exceeded

@CoMPaTech has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 23 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 1728cb2 and d510d14.

📒 Files selected for processing (1)
  • pyproject.toml (1 hunks)

Walkthrough

Centralizes HTTP handling in airos/airos8.py via _get_authenticated_headers, _api_call, and _request_json; refactors login/status/command flows to use them; adds firmware endpoints (update_check, download, progress, install); updates docs/changelog, bumps package version, and adapts tests to use session.request with adjusted mocks.

Changes

Cohort / File(s) Summary
Changelog & README
CHANGELOG.md, README.md
Added 0.4.0 entry (2025-08-16) and documented new airos.airos8 calls (warnings, provmode, update_check, download, progress, install) with example payloads; removed the "future releases" line.
Packaging
pyproject.toml
Bumped package version 0.3.00.4.0a2; adjusted description/keywords spelling to "Ubiquiti".
Module docstring
airos/__init__.py
Minor docstring wording/capitalization change.
Core refactor & API additions
airos/airos8.py
Added ApiResponse NamedTuple; added helpers _get_authenticated_headers, _api_call, _request_json; refactored login, status, stakick, provmode, warnings to use centralized request flow; added async firmware methods update_check(force), download(), progress(), install(); standardized CSRF/cookie handling and tightened warnings() signature.
Tests (airos8)
tests/test_airos8.py
Switched test mocks to use session.request for all HTTP calls; adjusted mock responses to expose .text; updated side_effects and assertions to match unified request flow and new firmware endpoints.
Tests (stations)
tests/test_stations.py
Switched to session.request, updated logging expectations to include request URL, adapted stakick/status mocks, disabled two tests, and cleaned imports.
Tests docstrings
tests/__init__.py, tests/conftest.py
Minor docstring wording updates (branding).

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AirOS
  participant HTTP

  Client->>AirOS: login(credentials)
  AirOS->>HTTP: _api_call(POST /login, payload)
  HTTP-->>AirOS: 200 + cookies + csrf token
  AirOS-->>Client: True

  Client->>AirOS: status()
  AirOS->>HTTP: _api_call(GET /status, auth headers)
  HTTP-->>AirOS: 200 + JSON
  AirOS-->>Client: AirOSData
Loading
sequenceDiagram
  participant Client
  participant AirOS
  participant HTTP

  Client->>AirOS: update_check(force?)
  AirOS->>HTTP: _api_call(POST /fw/update-check, payload/headers)
  HTTP-->>AirOS: 200 + JSON
  AirOS-->>Client: update info

  Client->>AirOS: download()
  AirOS->>HTTP: _api_call(POST /fw/download)
  HTTP-->>AirOS: 200 + JSON
  AirOS-->>Client: ack

  Client->>AirOS: progress()
  AirOS->>HTTP: _api_call(GET /fw/download-progress)
  HTTP-->>AirOS: 200 + JSON
  AirOS-->>Client: progress

  Client->>AirOS: install()
  AirOS->>HTTP: _api_call(POST /fw/install)
  HTTP-->>AirOS: 200 + JSON
  AirOS-->>Client: install result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

enhancement

Poem

A hop, a ping, I tidy the trail,
Cookies and tokens in my mail.
Check, download, progress in view,
Install the patch — all fresh and new.
A rabbit cheers this tidy release! 🐇

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch upd

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@codecov
Copy link

codecov bot commented Aug 16, 2025

Codecov Report

❌ Patch coverage is 82.23684% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.86%. Comparing base (a41436e) to head (264430e).

Files with missing lines Patch % Lines
airos/airos8.py 80.00% 27 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
- Coverage   95.15%   94.86%   -0.29%     
==========================================
  Files           9        9              
  Lines        1487     1384     -103     
==========================================
- Hits         1415     1313     -102     
+ Misses         72       71       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (11)
CHANGELOG.md (1)

5-12: Polish the 0.4.0 entry and align with version string

  • Grammar/style:
    • “Documentation on available class functions” → “Documentation of available class functions”
    • “Added the additional firmware update related functions” → “Added additional firmware‑update‑related functions”
  • Version alignment with pyproject (0.4.0a0) if intended as pre-release.

Proposed diff:

-## [0.4.0] - 2025-08-16
+## [0.4.0] - 2025-08-16
@@
-- Documentation on available class functions
-- Added the additional firmware update related functions
+- Documentation of available class functions
+- Added additional firmware-update-related functions

If you decide to reflect the alpha version:

-## [0.4.0] - 2025-08-16
+## [0.4.0a0] - 2025-08-16
airos/airos8.py (3)

147-159: Ensure mutually exclusive Content-Type selection

ct_json and ct_form are mutually exclusive. Today, if both are True, ct_form silently wins. Add a guard to avoid accidental conflicts.

Apply this diff:

 def _get_authenticated_headers(
     self, ct_json: bool = False, ct_form: bool = False
 ) -> dict[str, Any]:
     """Return common headers with CSRF token and optional Content-Type."""
     headers = {**self._common_headers}
     if self.current_csrf_token:
         headers["X-CSRF-ID"] = self.current_csrf_token
+    if ct_json and ct_form:
+        raise ValueError("ct_json and ct_form are mutually exclusive")
     if ct_json:
         headers["Content-Type"] = "application/json"
     if ct_form:
         headers["Content-Type"] = "application/x-www-form-urlencoded; charset=UTF-8"
     return headers

133-136: Nit: comment typo (“fist” → “first”)

Minor readability fix.

Apply this diff:

-        # Fallback take fist alternate interface found
+        # Fallback: take first alternate interface found

129-136: Defensive access for interface keys

Assuming presence of "enabled", "ifname", and "hwaddr" can raise KeyError on some devices/firmwares. Consider .get() with defaults to avoid hard failures and log when fields are missing.

I can provide a guarded variant if you want to harden this for partial interface dictionaries.

tests/test_stations.py (2)

179-183: Use side_effect to return distinct login and status responses

You’re returning the status response for both login and status calls. While current login passes due to permissive checks/mocks, it’s more realistic and robust to use a two-element side_effect.

Apply this diff:

-with (
-    patch.object(
-        airos_device.session, "request", return_value=mock_status_response
-    ),
-):
+with (
+    patch.object(
+        airos_device.session,
+        "request",
+        side_effect=[mock_login_response, mock_status_response],
+    ),
+):

201-203: Simplify AsyncMock assignment

You can inline the return value for clarity.

Apply this diff:

-    mock_stakick_response.text = AsyncMock()
-    mock_stakick_response.text.return_value = ""
+    mock_stakick_response.text = AsyncMock(return_value="")
README.md (2)

124-137: New API bullets are clear; minor grammar/consistency fixes suggested

Tweak wording for consistency with earlier bullets and to clarify the force parameter; also soften the parenthetical in progress().

Apply this diff:

-  - `warnings()`: Retrieves warning status dict.
+  - `warnings()`: Retrieves warning status dictionary.
...
-  - `update_check(force: bool = False)`: Checks if new firmware has been discovered (or force to force check).
+  - `update_check(force: bool = False)`: Checks whether new firmware is available (use force=True to force a check).
...
-  - `progress()`: Fetches the firmware download (not install!) progress.
+  - `progress()`: Fetches the firmware download progress (install progress is not reported).

151-154: Clarify progress description sentence

Small wording/punctuation tweak to make the sentence read naturally.

Apply this diff:

-If no progress to report ```{'progress': -1}``` otherwise a positive value between 0 and 100.
+If no progress is available, returns ```{"progress": -1}```; otherwise returns a value between 0 and 100.
tests/test_airos8.py (3)

195-197: Condense AsyncMock assignment for readability

Minor improvement to reduce two lines into one.

Apply these diffs:

-    mock_provmode_response.text = AsyncMock()
-    mock_provmode_response.text.return_value = ""
+    mock_provmode_response.text = AsyncMock(return_value="")
-    mock_provmode_response.text = AsyncMock()
-    mock_provmode_response.text.return_value = ""
+    mock_provmode_response.text = AsyncMock(return_value="")

Also applies to: 211-213


294-301: Optional: Use pathlib for robust fixture loading

Relative paths to fixtures can be brittle under different CWDs. Consider resolving paths from the test file directory.

Outside the selected lines, you could adapt like:

from pathlib import Path

FIXTURES = Path(__file__).parent / "fixtures"

async with aiofiles.open(FIXTURES / "warnings.json") as f:
    content = await f.read()
    mock_response_data = json.loads(content)

Also applies to: 346-352


121-134: Add tests for new firmware endpoints (download, progress, install)

Great coverage for update_check; consider adding minimal happy-path and error-path tests for download(), progress(), and install() to lock the new API surface.

I can draft tests mirroring the existing pattern (mocking session.request with 200/500 and invalid JSON where applicable). Want me to open a follow-up PR with these?

Also applies to: 281-357

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a41436e and 264430e.

📒 Files selected for processing (7)
  • CHANGELOG.md (1 hunks)
  • README.md (1 hunks)
  • airos/__init__.py (1 hunks)
  • airos/airos8.py (4 hunks)
  • pyproject.toml (1 hunks)
  • tests/test_airos8.py (16 hunks)
  • tests/test_stations.py (8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/test_airos8.py (1)
tests/conftest.py (1)
  • airos_device (23-28)
tests/test_stations.py (4)
airos/data.py (1)
  • AirOS8Data (576-593)
airos/exceptions.py (1)
  • AirOSDeviceConnectionError (24-25)
tests/conftest.py (1)
  • airos_device (23-28)
airos/airos8.py (1)
  • stakick (338-357)
airos/airos8.py (3)
tests/conftest.py (1)
  • base_url (17-19)
airos/exceptions.py (5)
  • AirOSDeviceConnectionError (24-25)
  • AirOSConnectionAuthenticationError (12-13)
  • AirOSDataMissingError (16-17)
  • AirOSConnectionSetupError (8-9)
  • AirOSKeyDataMissingError (20-21)
airos/data.py (3)
  • redact_data_smart (42-101)
  • AirOS8Data (576-593)
  • AirOSDataClass (107-110)
🪛 LanguageTool
CHANGELOG.md

[grammar] ~9-~9: There might be a mistake here.
Context: ... - Refactoring of the code (DRY-ing up) - Documentation on available class functio...

(QB_NEW_EN)


[grammar] ~10-~10: There might be a mistake here.
Context: ...cumentation on available class functions - Added the additional firmware update rel...

(QB_NEW_EN)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Run mypy
🔇 Additional comments (8)
pyproject.toml (1)

7-7: Version/tag consistency: 0.4.0a0 vs CHANGELOG’s 0.4.0

Project version is set to 0.4.0a0, but CHANGELOG.md introduces 0.4.0. If this is an alpha pre-release, consider reflecting that in the changelog (e.g., 0.4.0a0) or bump pyproject to 0.4.0 to match.

Would you like me to adjust CHANGELOG to 0.4.0a0 or bump the package version to 0.4.0 for consistency?

README.md (1)

166-184: Warnings section looks good

Example structure and keys are clear and consistent with the new API surface.

tests/test_airos8.py (6)

30-32: Good migration to session.request for login path and ClientError simulation

This aligns tests with the centralized HTTP layer and keeps error handling coverage intact.

Also applies to: 42-42


67-69: Status path refactor to session.request looks correct

Non-200 and invalid-JSON cases are preserved; mocking via request keeps coverage consistent with the refactor.

Also applies to: 85-87, 106-108


162-164: stakick tests updated to session.request and error path verified

Both non-200 and ClientError branches are covered and aligned with the new HTTP wrapper.

Also applies to: 173-173


199-200: Provmode tests correctly updated to patch session.request across success and error paths

Coverage preserves success, non-200, and ClientError behavior.

Also applies to: 215-215, 230-230, 240-240


302-302: warnings() and update_check() tests migrated to session.request — LGTM

Mocks and JSON parsing paths look correct, including invalid JSON branches.

Also applies to: 326-326, 353-353, 378-378


265-267: Exception chaining verified: the production code already raises AirOSKeyDataMissingError with from <cause> (see airos/airos8.py at lines 127, 328, 336). No changes needed.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
airos/airos8.py (1)

428-434: Bug: Install uses form headers but JSON body; send as form data instead

Headers indicate form-encoded, but payload is passed via json=. Use data=.

     async def install(self) -> dict[str, Any]:
         """Install new firmware."""
         request_headers = self._get_authenticated_headers(ct_form=True)
         payload: dict[str, Any] = {"do_update": 1}
         return await self._request_json(
-            "POST", self._install_url, headers=request_headers, json=payload
+            "POST", self._install_url, headers=request_headers, data=payload
         )
🧹 Nitpick comments (6)
airos/airos8.py (6)

159-171: Avoid double-setting Content-Type; make ct_json/ct_form mutually exclusive

If both flags are ever True, the last one wins silently. Enforce mutual exclusivity to prevent subtle header bugs.

Apply this diff:

 def _get_authenticated_headers(
     self, ct_json: bool = False, ct_form: bool = False
 ) -> dict[str, Any]:
     """Return common headers with CSRF token and optional Content-Type."""
     headers = {**self._common_headers}
     if self.current_csrf_token:
         headers["X-CSRF-ID"] = self.current_csrf_token
-    if ct_json:
-        headers["Content-Type"] = "application/json"
-    if ct_form:
-        headers["Content-Type"] = "application/x-www-form-urlencoded; charset=UTF-8"
+    # Ensure only one Content-Type is set
+    if ct_json and ct_form:
+        raise ValueError("ct_json and ct_form are mutually exclusive")
+    if ct_json:
+        headers["Content-Type"] = "application/json"
+    elif ct_form:
+        headers["Content-Type"] = "application/x-www-form-urlencoded; charset=UTF-8"
     return headers

172-191: Also catch asyncio.TimeoutError in network error handling

aiohttp raises asyncio.TimeoutError; catching only built-in TimeoutError may miss it depending on Python version. Include asyncio.TimeoutError explicitly.

         try:
             async with self.session.request(
                 method, url, headers=headers, **kwargs
             ) as response:
                 response_text = await response.text()
                 return ApiResponse(
                     status=response.status,
-                    headers=dict(response.headers),
+                    headers={k.lower(): v for k, v in response.headers.items()},
                     cookies=response.cookies,
                     url=response.url,
                     text=response_text,
                 )
-        except (TimeoutError, aiohttp.ClientError) as err:
+        except (asyncio.TimeoutError, TimeoutError, aiohttp.ClientError) as err:
             _LOGGER.exception("Error during API call to %s: %s", url, err)
             raise AirOSDeviceConnectionError from err

Also applies to: 192-195


205-225: Good: no re-read of body; centralized status handling

Using the buffered response.text and handling non-200/403 with a clear log is clean. The refactor removes the fragile dependency on the response object’s state. Consider logging a short snippet of the body on JSON decode errors for easier triage.

Apply this small improvement to include a truncated body in the exception log:

-        except json.JSONDecodeError as err:
-            _LOGGER.exception("JSON Decode Error in API response from %s", url)
-            raise AirOSDataMissingError from err
+        except json.JSONDecodeError as err:
+            _LOGGER.exception(
+                "JSON Decode Error in API response from %s: %s",
+                url,
+                response.text[:512],
+            )
+            raise AirOSDataMissingError from err

185-191: Make CSRF header lookup case-insensitive to avoid brittle logins

You convert headers to a plain dict, losing case-insensitive access. If the server returns a different case for X-CSRF-ID, login will fail to capture the token. Normalize header keys to lowercase and fetch 'x-csrf-id'.

-                    headers=dict(response.headers),
+                    headers={k.lower(): v for k, v in response.headers.items()},
-        new_csrf_token = response.headers.get("X-CSRF-ID")
+        new_csrf_token = response.headers.get("x-csrf-id")

Also applies to: 285-289


252-284: Redundant cookie_jar.update_cookies call

You already injected the cookie with update_cookies({...}, response.url). The second direct update with {morsel.key: morsel.value} is redundant and can lead to confusing duplication.

-                # Let's try the direct update of the key-value
-                self.session.cookie_jar.update_cookies({morsel.key: morsel.value})
+                # Direct second update not needed; the scoped update above suffices.

294-299: Cookie jar emptiness check is ineffective

CookieJar truthiness is always True; this condition likely never fires. Check emptiness explicitly.

-        if not self.session.cookie_jar:  # pragma: no cover
+        # Check for an empty jar explicitly; truthiness of CookieJar is always True
+        if not any(True for _ in self.session.cookie_jar):  # pragma: no cover
             _LOGGER.exception(
                 "COOKIE JAR IS EMPTY after login POST. This is a major issue."
             )
             raise AirOSConnectionSetupError from None
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 264430e and 22f1fb1.

📒 Files selected for processing (7)
  • README.md (1 hunks)
  • airos/__init__.py (1 hunks)
  • airos/airos8.py (4 hunks)
  • pyproject.toml (1 hunks)
  • tests/__init__.py (1 hunks)
  • tests/conftest.py (1 hunks)
  • tests/test_stations.py (9 hunks)
✅ Files skipped from review due to trivial changes (2)
  • tests/init.py
  • tests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (4)
  • pyproject.toml
  • airos/init.py
  • README.md
  • tests/test_stations.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
airos/airos8.py (4)
tests/conftest.py (1)
  • base_url (17-19)
airos/exceptions.py (5)
  • AirOSDeviceConnectionError (24-25)
  • AirOSConnectionAuthenticationError (12-13)
  • AirOSDataMissingError (16-17)
  • AirOSConnectionSetupError (8-9)
  • AirOSKeyDataMissingError (20-21)
airos/data.py (1)
  • redact_data_smart (42-101)
tests/test_airos8.py (3)
  • test_update_check_correctly_parses_json (317-340)
  • test_update_check_raises_exception_on_invalid_json (344-365)
  • test_status_missing_interface_key_data (88-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Run mypy
🔇 Additional comments (8)
airos/airos8.py (8)

33-41: Solid: ApiResponse eliminates aiohttp response leakage

Returning a typed, plain ApiResponse built inside the async context is the right call. It avoids use-after-context pitfalls and centralizes response shape. Nice.


313-319: LGTM: Login validates JSON and sets connection state

Validating that the login response is JSON before flipping connected=True is reasonable and consistent with your error handling strategy.


321-349: LGTM: Status flow with derived data and redacted error logging

  • Centralized headers + _request_json use is clean.
  • Redacting sensitive data before logging deserialization issues is great.
  • Error classes map well to MissingField/InvalidFieldValue.

359-367: LGTM: stakick flow and error logging

Form submission, 200-ok handling, and logging the failure with body are straightforward and correct.


379-386: LGTM: provmode flow mirrors stakick appropriately

Consistent headers, payload, and error handling.


388-393: LGTM: warnings via centralized JSON wrapper

Headers and call path are consistent with the refactor.


399-405: Thanks for fixing force payload to submit as form data

This resolves the earlier mismatch between Content-Type and body. The flow now correctly uses ct_form + data= when force=True.


420-427: LGTM: download uses JSON headers and JSON body

Consistent with the API style in this module.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
airos/airos8.py (1)

58-67: Bug: custom port in host is ignored when building base_url

If the caller passes a URL with a non-default port (e.g., http://192.168.1.3:8080), the computed base_url drops the port, causing all subsequent requests to target the wrong endpoint.

Apply this diff to preserve the port:

 parsed_host = urlparse(host)
 scheme = (
     parsed_host.scheme
     if parsed_host.scheme
     else ("https" if use_ssl else "http")
 )
-hostname = parsed_host.hostname if parsed_host.hostname else host
-
-self.base_url = f"{scheme}://{hostname}"
+hostname = parsed_host.hostname if parsed_host.hostname else host
+port = parsed_host.port
+netloc = f"{hostname}:{port}" if port else hostname
+
+self.base_url = f"{scheme}://{netloc}"
♻️ Duplicate comments (1)
airos/airos8.py (1)

428-434: Bug: Form-encoded install payload is sent via JSON

Headers are form-encoded, but the body is sent with json=. Switch to data= to match Content-Type.

     request_headers = self._get_authenticated_headers(ct_form=True)
     payload: dict[str, Any] = {"do_update": 1}
     return await self._request_json(
-        "POST", self._install_url, headers=request_headers, json=payload
+        "POST", self._install_url, headers=request_headers, data=payload
     )
🧹 Nitpick comments (4)
airos/airos8.py (4)

159-171: Guard against conflicting Content-Type flags in headers builder

Passing both ct_json and ct_form as True is ambiguous; add a simple guard to fail fast if both are set. Low-risk check that prevents subtle header/body mismatches.

 def _get_authenticated_headers(
     self, ct_json: bool = False, ct_form: bool = False
 ) -> dict[str, Any]:
     """Return common headers with CSRF token and optional Content-Type."""
     headers = {**self._common_headers}
     if self.current_csrf_token:
         headers["X-CSRF-ID"] = self.current_csrf_token
+    if ct_json and ct_form:
+        raise ValueError("ct_json and ct_form are mutually exclusive")
     if ct_json:
         headers["Content-Type"] = "application/json"
     if ct_form:
         headers["Content-Type"] = "application/x-www-form-urlencoded; charset=UTF-8"
     return headers

192-194: Also catch asyncio timeouts from aiohttp operations

aiohttp typically raises asyncio.TimeoutError rather than built-in TimeoutError. Add it to the exception tuple so timeouts are consistently mapped to AirOSDeviceConnectionError.

-except (TimeoutError, aiohttp.ClientError) as err:
+except (asyncio.TimeoutError, TimeoutError, aiohttp.ClientError) as err:
     _LOGGER.exception("Error during API call to %s: %s", url, err)
     raise AirOSDeviceConnectionError from err

141-153: Make interface scan resilient to missing 'enabled' key + fix comment typo

  • Accessing interface["enabled"] can KeyError if the field is absent. Use .get().
  • Typo: “fist” -> “first”.
-        for interface in interfaces:
-            if interface["enabled"]:  # Only consider if enabled
+        for interface in interfaces:
+            if interface.get("enabled"):  # Only consider if enabled
                 addresses[interface["ifname"]] = interface["hwaddr"]
 
-        # Fallback take fist alternate interface found
+        # Fallback: take first alternate interface found
         derived["mac"] = interfaces[0]["hwaddr"]
         derived["mac_interface"] = interfaces[0]["ifname"]

265-284: Avoid duplicate cookie_jar.update_cookies calls; update once with URL context

You add the cookie twice (first with url context, then again without). The second update is redundant and may override attributes. Keep the single, URL-scoped update for clarity and correctness.

                 self.session.cookie_jar.update_cookies(
                     {
                         morsel.key: morsel.output(header="")[len(morsel.key) + 1 :]
                         .split(";")[0]
                         .strip()
                     },
                     response.url,
                 )
-                # The update_cookies method can take a SimpleCookie morsel directly or a dict.
-                # The morsel.output method gives 'NAME=VALUE; Path=...; HttpOnly'
-                # We just need 'NAME=VALUE' or the morsel object itself.
-                # Let's use the morsel directly which is more robust.
-                # Alternatively: self.session.cookie_jar.update_cookies({morsel.key: morsel.value}) might work if it's simpler.
-                # Aiohttp's update_cookies takes a dict mapping name to value.
-                # To pass the full morsel with its attributes, we need to add it to the jar's internal structure.
-                # Simpler: just ensure the key-value pair is there for simple jar.
-
-                # Let's try the direct update of the key-value
-                self.session.cookie_jar.update_cookies({morsel.key: morsel.value})
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4ff90b8 and 1728cb2.

📒 Files selected for processing (2)
  • airos/airos8.py (4 hunks)
  • pyproject.toml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • pyproject.toml
🧰 Additional context used
🧬 Code Graph Analysis (1)
airos/airos8.py (4)
tests/conftest.py (1)
  • base_url (17-19)
airos/exceptions.py (5)
  • AirOSDeviceConnectionError (24-25)
  • AirOSConnectionAuthenticationError (12-13)
  • AirOSDataMissingError (16-17)
  • AirOSConnectionSetupError (8-9)
  • AirOSKeyDataMissingError (20-21)
airos/data.py (4)
  • redact_data_smart (42-101)
  • AirOS8Data (576-593)
  • AirOSDataClass (107-110)
  • InterfaceStatus (502-519)
tests/test_airos8.py (2)
  • test_update_check_correctly_parses_json (317-340)
  • test_update_check_raises_exception_on_invalid_json (344-365)
🔇 Additional comments (3)
airos/airos8.py (3)

33-41: ApiResponse NamedTuple is a solid choice

Great move switching to a NamedTuple for response data. Clear fields, immutability, and better type safety without leaking aiohttp response objects.


395-409: update_check(force=...) payload/body alignment looks correct

Force=True uses form-encoded data, otherwise JSON. Headers and body are consistent. Nice.


411-427: progress() and download() correctly use JSON body with JSON headers

Headers and body types are aligned; no content-type mismatch here.

@sonarqubecloud
Copy link

@CoMPaTech CoMPaTech merged commit 0d60727 into main Aug 16, 2025
12 checks passed
@CoMPaTech CoMPaTech deleted the upd branch August 16, 2025 12:22
This was referenced Aug 16, 2025
@coderabbitai coderabbitai bot mentioned this pull request Aug 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant