Dev/1.2.2 #32
Dev/1.2.2 #32
Conversation
…ройства по умолчанию на DESKTOP
…онных тестов с MockServer
|
Warning Rate limit exceeded@ink-developer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 30 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
WalkthroughIntroduces UserAgentPayload class with device/environment fields, propagates it through sync and handshake flows, updates CI/CD workflows for test automation, adds authentication documentation, changes default device type from "WEB" to "DESKTOP", and adds the app_version parameter. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Client as MaxClient
participant Sync as WebSocket Mixin
participant Handshake as Handshake Flow
participant Payload as SyncPayload
App->>Client: start() with UserAgentPayload
activate Client
Client->>Sync: _sync(user_agent)
deactivate Client
activate Sync
Sync->>Payload: Create SyncPayload with user_agent
activate Payload
Payload-->>Sync: SyncPayload instance
deactivate Payload
Sync->>Handshake: _handshake(user_agent)
deactivate Sync
activate Handshake
Handshake->>Handshake: Include user_agent in handshake payload
Handshake-->>Sync: Handshake response
deactivate Handshake
Sync-->>App: Authentication complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 1
🧹 Nitpick comments (4)
pytest.ini (1)
1-26: Duplicate pytest configuration withpyproject.toml.Both
pytest.iniandpyproject.toml(lines 90-102) define pytest options. This can lead to confusion and potential conflicts. Key differences:
pytest.inihas markers:unit,skip_ci(not inpyproject.toml)pytest.inihastimeout = 30(not inpyproject.toml)addoptsdiffer slightly (-ra --color=yesonly inpytest.ini)Consider consolidating into one location.
pyproject.tomlis the modern standard.pyproject.toml (1)
40-61: Duplicate pytest dependencies betweentestanddevgroups.Lines 41-44 and 57-60 both declare
pytest,pytest-asyncio,pytest-cov, andpytest-timeout. Consider havingdevreference thetestgroup to avoid duplication:🔎 Proposed fix
[dependency-groups] test = [ "pytest>=8.0.0", "pytest-asyncio>=0.24.0", "pytest-cov>=5.0.0", "pytest-timeout>=2.1.0", "flake8", "mypy", ] dev = [ + {include-group = "test"}, "furo>=2025.9.25", "ghp-import>=2.1.0", "mkdocs>=1.6.1", "mkdocs-material>=9.6.18", "mkdocstrings[python]>=0.30.0", "pre-commit>=4.3.0", "pydocstring>=0.2.1", "sphinx>=8.1.3", - "pytest>=8.0.0", - "pytest-asyncio>=0.24.0", - "pytest-cov>=5.0.0", - "pytest-timeout>=2.1.0", ].github/workflows/tests.yml (1)
39-59: Linting and type-checking are non-blocking.
continue-on-error: trueon flake8 (line 52) and mypy (line 59) means these checks won't fail the build. This is acceptable for gradual adoption but may allow quality regressions. Consider making critical flake8 errors (E9, F63, F7, F82) blocking.src/pymax/payloads.py (1)
76-90: Simplify the redundant default_factory lambda.The lambda explicitly passes all default values that are already defined in the
UserAgentPayloadclass fields (lines 42-53). SinceUserAgentPayloadalready has defaults for all fields, this is unnecessary code duplication and creates a maintenance burden—if defaults change inUserAgentPayload, they must be updated in two places.🔎 Proposed simplification
- user_agent: UserAgentPayload = Field( - default_factory=lambda: UserAgentPayload( - device_type=DEFAULT_DEVICE_TYPE, - locale=DEFAULT_LOCALE, - device_locale=DEFAULT_DEVICE_LOCALE, - os_version=DEFAULT_OS_VERSION, - device_name=DEFAULT_DEVICE_NAME, - header_user_agent=DEFAULT_USER_AGENT, - app_version=DEFAULT_APP_VERSION, - screen=DEFAULT_SCREEN, - timezone=DEFAULT_TIMEZONE, - client_session_id=DEFAULT_CLIENT_SESSION_ID, - build_number=DEFAULT_BUILD_NUMBER, - ), - ) + user_agent: UserAgentPayload = Field(default_factory=UserAgentPayload)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/tests.yml(1 hunks)README.md(1 hunks)examples/test.py(1 hunks)pyproject.toml(4 hunks)pytest.ini(1 hunks)redocs/source/clients.rst(1 hunks)src/pymax/core.py(1 hunks)src/pymax/mixins/websocket.py(7 hunks)src/pymax/payloads.py(2 hunks)src/pymax/static/constant.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
examples/test.py (2)
src/pymax/payloads.py (1)
UserAgentPayload(42-53)src/pymax/core.py (1)
MaxClient(44-318)
src/pymax/core.py (2)
src/pymax/mixins/websocket.py (1)
_sync(437-484)src/pymax/mixins/socket.py (1)
_sync(573-613)
src/pymax/mixins/websocket.py (2)
src/pymax/mixins/socket.py (1)
connect(95-129)src/pymax/payloads.py (1)
UserAgentPayload(42-53)
🔇 Additional comments (12)
src/pymax/static/constant.py (1)
12-12: Breaking change: default authentication method now uses phone-based login.Changing
DEFAULT_DEVICE_TYPEfrom"WEB"to"DESKTOP"alters the default authentication flow. Existing users relying on the previous default (QR-code/web auth) will now get phone-based authentication. Consider documenting this in release notes or a migration guide.redocs/source/clients.rst (1)
23-44: LGTM!The warning block clearly documents the critical
device_typeparameter and its two authentication modes with practical code examples. This aligns well with the constant change and helps users understand the authentication flow differences.pyproject.toml (3)
3-3: LGTM!Version bump from 1.2.1 to 1.2.2 appropriately reflects the new functionality and fixes.
104-118: LGTM!Coverage configuration is well-structured with appropriate exclusion patterns for protocol classes, abstract methods, and type-checking blocks.
90-102: Remove redundant pytest configuration frompyproject.toml.The repository has both
pytest.iniandpyproject.tomlwith duplicate pytest settings. Since pytest prioritizespytest.iniwhen both exist, thepyproject.tomlconfiguration is unused. Additionally,pyproject.tomlis inconsistent withpytest.ini—it's missing theunitandskip_cimarkers and lacks the-ra --color=yesaddopts andtimeout = 30settings defined inpytest.ini. Remove the[tool.pytest.ini_options]section frompyproject.tomlto eliminate confusion and keep configuration in a single file.Likely an incorrect or invalid review comment.
.github/workflows/tests.yml (3)
116-119: Integration tests are non-blocking.
continue-on-error: truemeans integration test failures won't block PRs. If this is intentional (e.g., tests are still stabilizing), consider adding a comment explaining why.
68-74: LGTM!Codecov integration with
fail_ci_if_error: falseis appropriate—coverage upload failures shouldn't block builds.
10-32: LGTM!Well-structured test matrix covering Python 3.10-3.13 with
fail-fast: falseto ensure all versions are tested even if one fails. Pip caching will improve CI performance.README.md (1)
57-90: Excellent documentation of authentication flows.The new authentication section clearly explains the critical role of
device_typeand provides comprehensive examples for both DESKTOP and WEB authentication flows. The use of warning blocks and code examples makes this very user-friendly.examples/test.py (1)
6-6: LGTM - Example correctly demonstrates the new API.The example properly showcases the DESKTOP authentication flow with explicit
device_typeandapp_versionparameters, aligning with the documentation and PR objectives.src/pymax/core.py (1)
298-298: LGTM - Correctly passes user_agent to sync.The change properly propagates the
user_agent(set fromheadersparameter at line 144) into the_sync()method, which now expects this parameter as shown inwebsocket.pyline 437.src/pymax/mixins/websocket.py (1)
69-100: LGTM - User agent propagation is well-implemented.The changes correctly thread
UserAgentPayloadthrough the connection, handshake, and sync flows:
connect()accepts an optionaluser_agentand provides a sensible default_handshake()incorporates the user agent into the session initialization payload_sync()passes the user agent toSyncPayload, ensuring authentication uses the correct device contextThe flow is consistent with changes in
core.pyandpayloads.py. The minor logging consolidations (lines 53, 172, 356) also improve readability.Also applies to: 102-118, 437-484
| - name: Start MockServer | ||
| run: | | ||
| git clone https://github.com/fresh-milkshake/gomax-prerelease.git | ||
| cd gomax-prerelease/mockserver | ||
| go mod download | ||
| go run cmd/server/main.go & | ||
| sleep 3 |
There was a problem hiding this comment.
Unpinned external repository and fragile server startup.
Two concerns:
- Unpinned repo: Cloning
gomax-prereleasewithout a specific commit/tag means CI can break unexpectedly if that repo changes. - Hardcoded sleep:
sleep 3is fragile; the server may not be ready in time, causing flaky tests.
🔎 Proposed improvements
- name: Start MockServer
run: |
- git clone https://github.com/fresh-milkshake/gomax-prerelease.git
+ git clone --depth 1 --branch v1.0.0 https://github.com/fresh-milkshake/gomax-prerelease.git
cd gomax-prerelease/mockserver
go mod download
go run cmd/server/main.go &
- sleep 3
+ # Wait for server to be ready
+ for i in {1..30}; do
+ curl -s http://localhost:8080/health && break || sleep 1
+ done🤖 Prompt for AI Agents
.github/workflows/tests.yml around lines 108 to 114: the workflow clones an
unpinned external repo and uses a fragile fixed sleep to wait for the mock
server; update the clone to fetch a specific tag or commit (or add the repo as a
submodule) to pin the version, and replace the hardcoded sleep with a readiness
loop that waits for the server to respond (e.g., poll an HTTP health endpoint
with a timeout and exit non‑zero on failure) so CI is deterministic and not
flaky.
|
крч пофиг на тесты |
Описание
Кратко, что делает этот PR. Например, добавляет новый метод, исправляет баг, улучшает документацию.
Тесты + вход по номеру телефона
Тип изменений
Связанные задачи / Issue
Ссылка на issue, если есть: #
Тестирование
Покажите пример кода, который проверяет изменения:
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.