Dev/1.2.4 #34
Dev/1.2.4 #34
Conversation
…анов и часовых поясов
…екущей веб-версии
📝 WalkthroughWalkthroughBumps version to 1.2.4 and adds ua-generator; restructures client/transport interfaces (BaseClient/BaseTransport/ClientProtocol), adds device-type and session handling, read/contact attach APIs, raw file support, UA/randomized constants, web-version utility, and extensive docs updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Socket as SocketMaxClient
participant Auth as Auth Server (TCP)
participant DB as Session DB (session.db)
participant WS as MaxClient (WebSocket)
Note over Socket,Auth: Phone-based auth via SocketMaxClient
Socket->>Auth: register/login (phone + device_type DESKTOP/ANDROID/IOS)
Auth-->>Socket: returns token
Socket->>DB: persist token (session_name / session.db)
Note over WS,DB: WebSocket fast-connect using persisted token
WS->>DB: read token (session_name)
WS->>Auth: connect WebSocket with token + headers (device_type WEB, UA)
Auth-->>WS: authenticated WS session
Note over WS: now use MaxClient features (messages, contacts, change_profile)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/pymax/files.py (3)
12-23: Critical: Validation logic prevents raw-only initialization.The validation at lines 19-23 requires either
urlorpathto be provided, but the newrawparameter should allow initialization with raw bytes alone. Currently,BaseFile(raw=b"...")will raiseValueError("Either url or path must be provided."), making the raw-bytes feature unusable.🔎 Proposed fix
def __init__( self, raw: bytes | None = None, *, url: str | None = None, path: str | None = None ) -> None: self.raw = raw self.url = url self.path = path - if self.url is None and self.path is None: + if self.url is None and self.path is None and self.raw is None: raise ValueError("Either url or path must be provided.") if self.url and self.path: raise ValueError("Only one of url or path must be provided.")Consider also validating that only one of
raw,url, orpathis provided to prevent ambiguous initialization.
94-105: Critical: Cannot initialize Video with raw-only data.The validation at lines 103-104 raises
ValueErroriffile_nameis empty, butfile_namewill be empty when onlyrawis provided (withoutpathorurl). This makesVideo(raw=b"...")completely unusable and contradicts the PR objective of supporting raw byte initialization.🔎 Proposed fix
def __init__( self, raw: bytes | None = None, *, url: str | None = None, path: str | None = None ) -> None: - self.file_name: str = "" + self.file_name: str = "video" # default for raw bytes if path: self.file_name = Path(path).name elif url: self.file_name = Path(url).name - if not self.file_name: - raise ValueError("Either url or path must be provided.") super().__init__(raw=raw, url=url, path=path)Consider accepting an optional
nameparameter for better control over raw-bytes file names.
113-125: Critical: Cannot initialize File with raw-only data.The validation at lines 122-123 raises
ValueErroriffile_nameis empty, butfile_namewill be empty when onlyrawis provided. This makesFile(raw=b"...")unusable, identical to the issue inVideo.__init__.🔎 Proposed fix
def __init__( self, raw: bytes | None = None, *, url: str | None = None, path: str | None = None ) -> None: - self.file_name: str = "" + self.file_name: str = "file" # default for raw bytes if path: self.file_name = Path(path).name elif url: self.file_name = Path(url).name - if not self.file_name: - raise ValueError("Either url or path must be provided.") - super().__init__(raw=raw, url=url, path=path)Consider accepting an optional
nameparameter for better control over raw-bytes file names.src/pymax/mixins/websocket.py (1)
131-133: GenericRuntimeErrorloses exception context.Raising
RuntimeError("Send and wait failed")discards the original exception. Consider usingraise ... fromor re-raising the original exception to preserve the traceback.🔎 Proposed fix
except Exception: self.logger.exception("Send and wait failed (opcode=%s, seq=%s)", opcode, msg["seq"]) - raise RuntimeError("Send and wait failed") + raise finally: self._pending.pop(msg["seq"], None)src/pymax/mixins/socket.py (1)
85-87: Sequence number overflow will cause unhandled OverflowError.
_seqincrements without bound (interfaces.py line 188), but seq is packed into a single byte at line 87. After 255 messages,seq.to_bytes(1, "big")will raiseOverflowError. The exception is not caught in the try-except block (lines 253-274), which only handles SSL and connection errors. The protocol does not implement wrapping behavior (no modulo logic found), and_seqis not reset on reconnection. This will crash the send operation after 256 messages.
🧹 Nitpick comments (13)
src/pymax/static/constant.py (1)
78-86: Module-level randomization may not provide per-client diversity.Lines 78, 81, 84-85 use
choice()andrandint()at module initialization, which means all client instances in the same Python process will share the same "random" values. If the intent is to have different user-agents per client session, consider moving this randomization to instance initialization rather than module constants.Regarding the static analysis S311 warnings: these are false positives in this context since the random values are for user-agent diversity, not cryptographic security.
Example approach for per-client randomization
If per-client diversity is desired, consider making these factory functions rather than constants, or move the randomization to client initialization:
def get_default_device_name() -> str: return choice(DEVICE_NAMES) def get_default_os_version() -> str: return choice(OS_VERSIONS) # etc.Then use these functions during client instantiation instead of at module load time.
src/pymax/mixins/socket.py (2)
15-17: Unused import:ClientProtocol.
ClientProtocolis imported but not used directly in this file.SocketMixininherits fromBaseTransport, which already extendsClientProtocol.🔎 Proposed fix
from pymax.interfaces import BaseTransport from pymax.payloads import BaseWebSocketMessage, SyncPayload, UserAgentPayload -from pymax.protocols import ClientProtocol from pymax.static.constant import (
108-116: ANSI escape codes in log message.The warning message contains ANSI escape codes (
\033[0;31m) which won't render properly in log files or non-TTY environments. Consider using plain text or the logger's built-in formatting.🔎 Proposed fix
if sys.version_info[:2] == (3, 12): self.logger.warning( """ =============================================================== - ⚠️⚠️ \033[0;31mWARNING: Python 3.12 detected!\033[0m ⚠️⚠️ + ⚠️⚠️ WARNING: Python 3.12 detected! ⚠️⚠️ Socket connections may be unstable, SSL issues are possible. =============================================================== """ )src/pymax/mixins/websocket.py (2)
26-27: Exception raised as class, not instance.
WebSocketNotConnectedErroris raised as a class reference instead of an instance. This works but is inconsistent with Python conventions and the exception's__init__that sets the message.🔎 Proposed fix
if self._ws is None or not self.is_connected: self.logger.critical("WebSocket not connected when access attempted") - raise WebSocketNotConnectedError + raise WebSocketNotConnectedError() return self._ws
88-90: Exception set as class, not instance.Same issue as above -
WebSocketNotConnectedErrorshould be instantiated when setting as exception on futures.🔎 Proposed fix
for fut in self._pending.values(): if not fut.done(): - fut.set_exception(WebSocketNotConnectedError) + fut.set_exception(WebSocketNotConnectedError()) self._pending.clear()src/pymax/utils.py (1)
69-70: Unused variablee(static analysis hint).The exception variable
eis assigned but never used.🔎 Proposed fix
try: main_chunk_code = requests.get(main_chunk_url, timeout=10).text - except requests.exceptions.RequestException as e: + except requests.RequestException: return Nonesrc/pymax/protocols.py (1)
94-117: Type inconsistency:opcodeparameter types differ between abstract methods.
_send_and_waitusesopcode: Opcode(enum), while_queue_messageusesopcode: int. This inconsistency could lead to type confusion. Consider usingOpcodeconsistently.🔎 Proposed fix
@abstractmethod async def _queue_message( self, - opcode: int, + opcode: Opcode, payload: dict[str, Any], cmd: int = 0, timeout: float = DEFAULT_TIMEOUT, max_retries: int = 3, ) -> Message | None: passsrc/pymax/core.py (2)
41-41: Mutable class attributes should useClassVar.Static analysis correctly identifies that mutable class attributes (
allowed_device_types) should be annotated withtyping.ClassVarto indicate they are not instance attributes.🔎 Proposed fix
+from typing import ClassVar + class MaxClient(ApiMixin, WebSocketMixin, BaseClient): - allowed_device_types: set[str] = {"WEB"} + allowed_device_types: ClassVar[set[str]] = {"WEB"} ... class SocketMaxClient(SocketMixin, MaxClient): - allowed_device_types = {"ANDROID", "IOS", "DESKTOP"} + allowed_device_types: ClassVar[set[str]] = {"ANDROID", "IOS", "DESKTOP"}Also applies to: 334-334
306-318: Unused variablesdoneande(static analysis hints).The
doneset fromasyncio.waitand exception variableeare assigned but never used.🔎 Proposed fix
- done, pending = await asyncio.wait( + _, pending = await asyncio.wait( [wait_task, stop_task], return_when=asyncio.FIRST_COMPLETED ) ... - except Exception as e: + except Exception: self.logger.exception("Client start iteration failed")src/pymax/interfaces.py (4)
317-378: Optional: Simplify exception logging.The reaction, chat update, and raw receive handlers are well-implemented. However, Lines 344, 362, and 371 include redundant exception objects in
logging.exception()calls. Theexception()method already captures exception info automatically.🔎 Proposed fix
- self.logger.exception("Error in on_reaction_change_handler: %s", e) + self.logger.exception("Error in on_reaction_change_handler")- self.logger.exception("Error in on_chat_update_handler: %s", e) + self.logger.exception("Error in on_chat_update_handler")- self.logger.exception("Error in on_raw_receive_handler: %s", e) + self.logger.exception("Error in on_raw_receive_handler")
380-386: Optional: Simplify exception logging.The task exception logging helper works correctly, but Line 386 includes a redundant exception object in the
logging.exception()call, which automatically captures exception info.🔎 Proposed fix
- self.logger.exception("Error retrieving task exception: %s", e) + self.logger.exception("Error retrieving task exception")
428-428: TODO: Consider persistent message queue.The comment suggests implementing a persistent message queue. This could improve reliability by surviving client restarts.
Would you like me to open an issue to track this enhancement?
508-509: Remove unused variable assignment.The
errorvariable is assigned on Line 508 but never used. The walrus operator captures the error value, but it's immediately passed toMixinsUtils.handle_error(), making the local binding unnecessary.🔎 Proposed fix
- if error := raw_payload.get("error"): + if raw_payload.get("error"): MixinsUtils.handle_error(data)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
pyproject.tomlredocs/source/clients.rstredocs/source/conf.pyredocs/source/examples.rstredocs/source/guides.rstredocs/source/index.rstredocs/source/quickstart.rstredocs/source/release_notes.rstsrc/pymax/core.pysrc/pymax/files.pysrc/pymax/interfaces.pysrc/pymax/mixins/auth.pysrc/pymax/mixins/channel.pysrc/pymax/mixins/group.pysrc/pymax/mixins/handler.pysrc/pymax/mixins/message.pysrc/pymax/mixins/scheduler.pysrc/pymax/mixins/self.pysrc/pymax/mixins/socket.pysrc/pymax/mixins/telemetry.pysrc/pymax/mixins/user.pysrc/pymax/mixins/utils.pysrc/pymax/mixins/websocket.pysrc/pymax/payloads.pysrc/pymax/protocols.pysrc/pymax/static/constant.pysrc/pymax/static/enum.pysrc/pymax/types.pysrc/pymax/utils.py
💤 Files with no reviewable changes (3)
- redocs/source/quickstart.rst
- redocs/source/guides.rst
- src/pymax/mixins/utils.py
🧰 Additional context used
🧬 Code graph analysis (14)
src/pymax/utils.py (1)
src/pymax/exceptions.py (2)
Error(60-86)RateLimitError(89-97)
src/pymax/mixins/telemetry.py (3)
src/pymax/protocols.py (1)
ClientProtocol(32-123)src/pymax/static/enum.py (1)
Opcode(4-146)src/pymax/payloads.py (1)
NavigationEventPayload(258-263)
src/pymax/payloads.py (1)
src/pymax/static/enum.py (1)
ReadAction(221-223)
src/pymax/mixins/channel.py (3)
src/pymax/protocols.py (1)
ClientProtocol(32-123)src/pymax/types.py (2)
Channel(942-949)Member(167-213)src/pymax/utils.py (1)
MixinsUtils(11-90)
src/pymax/mixins/group.py (2)
src/pymax/protocols.py (1)
ClientProtocol(32-123)src/pymax/utils.py (1)
MixinsUtils(11-90)
src/pymax/mixins/scheduler.py (1)
src/pymax/protocols.py (1)
ClientProtocol(32-123)
src/pymax/protocols.py (5)
src/pymax/payloads.py (1)
UserAgentPayload(42-53)src/pymax/static/enum.py (1)
Opcode(4-146)src/pymax/interfaces.py (1)
_send_and_wait(174-180)src/pymax/mixins/socket.py (1)
_send_and_wait(238-290)src/pymax/mixins/websocket.py (1)
_send_and_wait(103-135)
src/pymax/mixins/auth.py (3)
src/pymax/protocols.py (1)
ClientProtocol(32-123)src/pymax/static/enum.py (3)
AuthType(173-177)DeviceType(186-189)Opcode(4-146)src/pymax/utils.py (1)
MixinsUtils(11-90)
src/pymax/mixins/self.py (2)
src/pymax/protocols.py (1)
ClientProtocol(32-123)src/pymax/utils.py (1)
MixinsUtils(11-90)
src/pymax/mixins/message.py (4)
src/pymax/payloads.py (1)
ReadMessagesPayload(363-367)src/pymax/static/enum.py (2)
Opcode(4-146)ReadAction(221-223)src/pymax/types.py (18)
Message(663-765)ReadState(1198-1220)from_dict(34-35)from_dict(72-78)from_dict(146-156)from_dict(190-205)from_dict(246-260)from_dict(284-292)from_dict(323-332)from_dict(367-376)from_dict(415-426)from_dict(452-459)from_dict(483-487)from_dict(502-511)from_dict(532-540)from_dict(558-559)from_dict(577-582)from_dict(601-602)src/pymax/interfaces.py (1)
_send_and_wait(174-180)
src/pymax/mixins/socket.py (3)
src/pymax/interfaces.py (1)
BaseTransport(167-552)src/pymax/payloads.py (3)
BaseWebSocketMessage(34-39)SyncPayload(68-90)UserAgentPayload(42-53)src/pymax/protocols.py (1)
ClientProtocol(32-123)
src/pymax/core.py (2)
src/pymax/exceptions.py (1)
WebSocketNotConnectedError(13-20)src/pymax/payloads.py (1)
UserAgentPayload(42-53)
src/pymax/mixins/websocket.py (5)
src/pymax/exceptions.py (1)
WebSocketNotConnectedError(13-20)src/pymax/interfaces.py (1)
BaseTransport(167-552)src/pymax/payloads.py (1)
UserAgentPayload(42-53)src/pymax/static/enum.py (1)
Opcode(4-146)src/pymax/types.py (1)
Chat(837-939)
src/pymax/mixins/handler.py (1)
src/pymax/protocols.py (1)
ClientProtocol(32-123)
🪛 Ruff (0.14.10)
src/pymax/utils.py
69-69: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
src/pymax/static/constant.py
78-78: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
81-81: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
84-84: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
85-85: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
src/pymax/core.py
41-41: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
43-43: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
193-196: Avoid specifying long messages outside the exception class
(TRY003)
306-306: Unpacked variable done is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
318-318: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
334-334: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
src/pymax/interfaces.py
209-209: Do not catch blind exception: Exception
(BLE001)
251-251: Do not catch blind exception: Exception
(BLE001)
344-344: Redundant exception object included in logging.exception call
(TRY401)
362-362: Redundant exception object included in logging.exception call
(TRY401)
371-371: Redundant exception object included in logging.exception call
(TRY401)
386-386: Redundant exception object included in logging.exception call
(TRY401)
444-444: Do not catch blind exception: Exception
(BLE001)
469-472: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
508-508: Local variable error is assigned to but never used
(F841)
546-546: Use raise without specifying exception name
Remove exception name
(TRY201)
🔇 Additional comments (43)
src/pymax/files.py (1)
27-28: The short-circuit implementation is correct and does not bypass validation. Photo objects require eitherurlorpathperBaseFile.__init__()constraints (lines 19-23), andvalidate_photo()validates based on these required attributes, not raw bytes. Therawparameter is simply an optional optimization to avoid redundant I/O operations when bytes are already available upstream.Likely an incorrect or invalid review comment.
redocs/source/examples.rst (2)
41-41: Verify the filter change is intentional and update the example description if needed.The Greeter Bot example changed from
Filters.private()toFilters.chat(123), which fundamentally alters the behavior—it now only greets in chat ID 123 rather than all private chats. The example description still says "Приветствует новых пользователей" (Greets new users) without mentioning the chat-specific restriction.
153-154: Simplified file attachment example looks good.The updated example now directly prints
attach.base_urlinstead of making an additional API call, which is cleaner and more straightforward for documentation purposes.src/pymax/static/constant.py (3)
1-1: Imports look good.The addition of
choiceandrandintfrom the random module is appropriate for the randomization logic introduced in this file.
10-66: New constant collections added for user-agent diversification.The addition of DEVICE_NAMES, SCREEN_SIZES, OS_VERSIONS, and TIMEZONES provides a good foundation for generating diverse user agents and avoiding fingerprinting.
82-82: The concerns aboutua_generator.generate()are not applicable to this codebase. Theua-generatorlibrary is a lightweight, pure-Python utility that performs only in-memory string construction with no I/O, no external dependencies, and no special initialization requirements. Calling it at module import time has negligible performance impact. Additionally, whileDEFAULT_USER_AGENTis a module-level constant shared across the process, this is intentional design—the value is used as a default in Pydantic models and can be overridden on a per-request basis. The pattern is consistent with other constants in the file (e.g.,DEFAULT_OS_VERSION,DEFAULT_CLIENT_SESSION_ID).Likely an incorrect or invalid review comment.
src/pymax/mixins/self.py (2)
19-19: Import path update aligns with architectural refactoring.The ClientProtocol import has been correctly updated from
pymax.interfacestopymax.protocolsas part of the broader protocol reorganization in this PR.
22-22: MixinsUtils import path updated correctly.The import has been correctly updated from
pymax.mixins.utilstopymax.utils, aligning with the utility module reorganization.src/pymax/mixins/scheduler.py (1)
6-6: Import path update is correct.The ClientProtocol import has been properly updated to use
pymax.protocols, consistent with the architectural changes across the codebase.src/pymax/mixins/handler.py (2)
5-5: Import path correctly updated.The ClientProtocol import has been properly updated to
pymax.protocols, consistent with the architectural refactoring.
65-65: Minor formatting improvements enhance readability.The consolidation of multi-line statements to single lines (debug logging and task scheduling) improves code conciseness without affecting functionality.
Also applies to: 89-89, 178-178
src/pymax/mixins/auth.py (2)
11-11: Import path update is correct.The ClientProtocol import has been properly updated from
pymax.interfacestopymax.protocols, aligning with the protocol reorganization.
14-14: MixinsUtils import path updated correctly.The import has been correctly updated from
pymax.mixins.utilstopymax.utils, consistent with the utility module reorganization across the codebase.pyproject.toml (1)
22-22: ua-generator 2.0.19 is the latest stable release with no known security vulnerabilities.src/pymax/payloads.py (1)
18-18: LGTM! New payload model follows established patterns.The ReadMessagesPayload addition is consistent with other payload models in the file and properly integrates with the new ReadAction enum.
Also applies to: 361-367
redocs/source/index.rst (1)
38-49: Documentation updates appropriately reflect new features.The addition of release notes and feature descriptions aligns well with the new functionality introduced in this PR.
src/pymax/mixins/user.py (1)
9-9: Import refactoring aligns with new module structure.The updates to import paths (ClientProtocol from pymax.protocols, MixinsUtils from pymax.utils) are consistent with the broader architectural refactoring in this PR.
Also applies to: 12-12, 125-125
src/pymax/types.py (1)
1196-1220: Well-structured type class following project conventions.The ReadState class implementation is consistent with other type classes in the file, with proper initialization, serialization, and string representation methods.
src/pymax/mixins/channel.py (1)
8-8: Import updates consistent with architectural refactoring.Changes align with the project-wide module restructuring, moving to the new pymax.protocols and pymax.utils imports.
Also applies to: 15-15, 116-116
src/pymax/mixins/group.py (1)
19-19: Import refactoring matches project-wide pattern.The import path updates are consistent with the architectural changes moving ClientProtocol and MixinsUtils to their new module locations.
Also applies to: 22-22
src/pymax/static/enum.py (1)
219-223: New enum follows established conventions.The ReadAction enum is properly structured and consistent with other enum definitions in the file.
src/pymax/mixins/telemetry.py (1)
12-12: Refactoring aligns with new module structure.Import and formatting updates are consistent with the broader architectural changes in this PR.
Also applies to: 17-17
redocs/source/clients.rst (1)
87-111: LGTM! Documentation for new API methods is clear and consistent.The new documentation for
change_profile(),resolve_group_by_link(), andclient.contactsproperty follows the existing style and provides useful examples.src/pymax/mixins/socket.py (1)
276-284: Reconnection logic in_send_and_waitis reasonable but note recursion risk.The reconnection attempt within
_send_and_waitis appropriate, and re-raisingSocketNotConnectedErrorafter the attempt ensures the caller can handle the failure. However, be aware that rapid consecutive failures could cause stack depth issues if callers retry immediately.src/pymax/mixins/message.py (2)
853-879: Newread_messagemethod looks good, but missing payload validation.The implementation is clean and follows the existing patterns. However, line 879 accesses
data["payload"]directly which could raiseKeyErrorif the response is malformed. Consider using.get("payload", {})with appropriate fallback handling.🔎 Suggested defensive fix
self.logger.debug("read_message success") - return ReadState.from_dict(data["payload"]) + payload = data.get("payload") + if not payload: + raise Error("no_payload", "Payload missing in response", "Read Error") + return ReadState.from_dict(payload)
26-45: Import additions forread_messagefunctionality are appropriate.The new imports (
ReadMessagesPayload,ReadAction,ReadState,MixinsUtils) are all used by the newread_messagemethod and related error handling.redocs/source/release_notes.rst (1)
1-2: Version mismatch between PR and release notes.The PR is titled "Dev/1.2.4" but the release notes document version 1.2.3. Please verify if this is intentional (documenting the previous release) or if the version should be updated to 1.2.4.
Also applies to: 58-61
src/pymax/protocols.py (1)
32-92: Well-structured abstract protocol class.The
ClientProtocolABC properly defines the contract for client implementations with comprehensive state initialization and clear abstract method signatures. The use ofTYPE_CHECKINGfor conditional imports is a good practice.src/pymax/core.py (2)
275-330: Improved lifecycle management with_stop_event.The refactored
start()method using_stop_eventfor graceful shutdown is a significant improvement over direct task cancellation. The reconnection loop with proper cleanup is well-structured.
187-196: Device type validation is a good security/correctness addition.The
_validate_device_type()method ensures that clients only use supported device types, preventing configuration errors. The separateallowed_device_typesforMaxClient(WEB) andSocketMaxClient(ANDROID, IOS, DESKTOP) aligns with the authentication requirements documented inclients.rst.src/pymax/utils.py (1)
58-90: This function appears to be unused and should be removed or refactored.
get_current_web_version()is currently not called anywhere in the codebase. However, if it is intended for future use, the synchronousrequests.get()calls (lines 61, 68) are incompatible with the async architecture used throughout the rest of the codebase. Either remove this method if it's dead code, or convert it to useaiohttplike the rest of the project.src/pymax/interfaces.py (12)
1-32: LGTM!The imports are well-organized and include all necessary dependencies for the abstract base classes and WebSocket handling implementation.
36-46: LGTM!The lazy logger setup with ColoredFormatter is well-implemented with proper guards against duplicate handler registration.
48-69: LGTM!The safe execution wrappers provide good error isolation for background tasks with proper exception handling and lifecycle tracking.
71-107: LGTM!The cleanup logic is comprehensive and properly handles task cancellation, pending futures, and WebSocket closure with appropriate exception handling for cleanup contexts.
109-144: LGTM!The utility methods and async context manager implementation are correct. The
idle()method properly uses an Event for indefinite waiting, and the inspect method provides useful debugging output.
146-164: LGTM!The abstract method declarations properly define the contract for subclasses implementing the client lifecycle.
167-198: LGTM!The abstract transport methods and message creation logic are well-implemented with proper sequence tracking and debug logging.
200-229: LGTM!The interactive ping loop and handshake implementation are correct. The broad exception handling in the ping loop (Line 209) is appropriate for a keep-alive mechanism that should not crash the client.
231-262: LGTM!The message handler processing, JSON parsing, and pending response matching logic are correctly implemented with appropriate error handling.
264-315: LGTM!The queue handling, file upload notifications, and message notification processing are well-structured with proper status-based handler dispatch.
478-486: LGTM!The retry delay logic properly differentiates between error types with appropriate backoff strategies.
488-552: LGTM on overall sync implementation!The sync method properly populates all entity collections (dialogs, chats, channels, contacts, profile) from the LOGIN response with comprehensive error handling and connection cleanup.
| if self._error_count > 10: | ||
| self._circuit_breaker = True | ||
| self.logger.warning( | ||
| "Circuit breaker activated due to %d consecutive errors", | ||
| self._error_count, | ||
| ) | ||
| await self._outgoing.put(message) | ||
| continue |
There was a problem hiding this comment.
Review circuit breaker re-queue logic.
When the circuit breaker activates (line 449), the message is re-queued (line 454) without incrementing retry_count or checking max_retries. If errors persist, this could lead to the same message being re-queued indefinitely once the breaker resets every 60 seconds.
Consider either dropping the message after the breaker activates a certain number of times, or incrementing the retry count before re-queuing.
🔎 Proposed fix to respect max_retries
if self._error_count > 10:
self._circuit_breaker = True
self.logger.warning(
"Circuit breaker activated due to %d consecutive errors",
self._error_count,
)
+ if retry_count < max_retries:
+ message["retry_count"] = retry_count + 1
- await self._outgoing.put(message)
+ await self._outgoing.put(message)
+ else:
+ self.logger.error("Message dropped: circuit breaker active and max retries exceeded")
continueCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/pymax/interfaces.py around lines 448 to 455, the circuit breaker path
currently re-queues the message without updating retry state, risking endless
re-delivery; modify this block to increment the message's retry_count
(initialize to 0 if missing) and check against max_retries before re-queuing: if
retry_count >= max_retries, drop the message and log a warning/error; otherwise
increment retry_count, optionally record last_retry_time, then put the message
back on _outgoing; ensure any persistence or ack semantics used elsewhere are
respected when dropping vs re-queuing.
| self.logger.error( | ||
| "Message failed after %d retries, dropping", | ||
| max_retries, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use logging.exception for consistency.
This error block should use logging.exception instead of logging.error to automatically capture the exception traceback, consistent with other error handling in this file.
🔎 Proposed fix
- self.logger.error(
- "Message failed after %d retries, dropping",
- max_retries,
- )
+ self.logger.exception(
+ "Message failed after %d retries, dropping: %d",
+ max_retries,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.error( | |
| "Message failed after %d retries, dropping", | |
| max_retries, | |
| ) | |
| self.logger.exception( | |
| "Message failed after %d retries, dropping", | |
| max_retries, | |
| ) |
🧰 Tools
🪛 Ruff (0.14.10)
469-472: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In src/pymax/interfaces.py around lines 469 to 472, the error logging uses
self.logger.error which does not capture the exception traceback; replace it
with self.logger.exception and pass the same formatted message and max_retries
so the exception traceback is logged consistently with other error handling in
this file.
| main_chunk_import = html.split("import(")[2].split(")")[0].strip("\"'") | ||
| main_chunk_url = f"https://web.max.ru{main_chunk_import}" | ||
| try: | ||
| main_chunk_code = requests.get(main_chunk_url, timeout=10).text | ||
| except requests.exceptions.RequestException as e: | ||
| return None | ||
|
|
||
| arr = main_chunk_code.split("\n")[0].split("[")[1].split("]")[0].split(",") |
There was a problem hiding this comment.
Fragile HTML/JS parsing prone to IndexError.
The parsing logic uses hardcoded indices (split("import(")[2], split("[")[1]) which will raise IndexError if the page structure changes. Consider wrapping in try/except or using more robust parsing.
🔎 Proposed fix
+ @staticmethod
+ def get_current_web_version() -> str | None:
try:
html = requests.get("https://web.max.ru/", timeout=10).text
except requests.RequestException:
return None
- main_chunk_import = html.split("import(")[2].split(")")[0].strip("\"'")
+ try:
+ main_chunk_import = html.split("import(")[2].split(")")[0].strip("\"'")
+ except IndexError:
+ return None
+
main_chunk_url = f"https://web.max.ru{main_chunk_import}"
try:
main_chunk_code = requests.get(main_chunk_url, timeout=10).text
- except requests.exceptions.RequestException as e:
+ except requests.RequestException:
return None
- arr = main_chunk_code.split("\n")[0].split("[")[1].split("]")[0].split(",")
+ try:
+ arr = main_chunk_code.split("\n")[0].split("[")[1].split("]")[0].split(",")
+ except IndexError:
+ return NoneCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
69-69: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🤖 Prompt for AI Agents
In src/pymax/utils.py around lines 65 to 72, the chain of hardcoded splits
(e.g., split("import(")[2], split("[")[1]) is fragile and will raise IndexError
if the HTML/JS shape changes; update the code to defensively parse by first
checking that each split yields the expected number of parts (or use a single
robust regex to extract the import(...) argument and the bracketed list), wrap
the parsing in a try/except that catches IndexError (and ValueError) in addition
to the existing RequestException, return None on failure, and optionally log the
parsing failure with context so callers can diagnose malformed pages.
…ocs, улучшение документации и добавление новых функций в релиз 1.2.4
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pymax/files.py (1)
12-23:BaseFilevalidation contradicts raw-only initialization intent.The constructor requires either
urlorpath(lines 19-23), but theread()method (lines 27-29) is designed to short-circuit whenrawis provided. This meansPhoto(raw=b"...")will fail at validation, even though theread()method would handle it correctly.If raw-only initialization should be supported:
🔎 Proposed fix
def __init__( self, raw: bytes | None = None, *, url: str | None = None, path: str | None = None ) -> None: self.raw = raw self.url = url self.path = path - if self.url is None and self.path is None: + if self.raw is None and self.url is None and self.path is None: raise ValueError("Either url or path must be provided.") if self.url and self.path: raise ValueError("Only one of url or path must be provided.")
♻️ Duplicate comments (2)
src/pymax/interfaces.py (2)
448-455: Circuit breaker re-queues message without incrementing retry count.When the circuit breaker activates, the message is re-queued (line 454) without incrementing
retry_count. This could lead to indefinite re-queueing once the breaker resets.
469-472: Uselogging.exceptioninstead oflogging.error.This error block should use
logging.exceptionto capture the exception traceback, consistent with other error handling in this file. Static analysis also flags this (TRY400).
🧹 Nitpick comments (3)
src/pymax/files.py (1)
54-71: Emptyfile_namefallback may cause downstream issues.While the
nameparameter addition addresses the previous review concern aboutAttributeError, defaulting to an empty string (line 69) may cause problems with file upload APIs that expect meaningful filenames.Consider using a sensible default like
"photo"or requiringnamewhen using raw bytes:🔎 Proposed improvement
elif name: self.file_name = name else: - self.file_name = "" + self.file_name = "photo"src/pymax/interfaces.py (2)
338-345: Minor: Redundant exception object inlogging.exception.The exception object
eis passed explicitly butlogging.exceptionalready captures it automatically. This pattern repeats at lines 344, 362, 371, and 386.🔎 Proposed fix
except Exception as e: - self.logger.exception("Error in on_reaction_change_handler: %s", e) + self.logger.exception("Error in on_reaction_change_handler")
504-546: Minor: Unused assigned variables.
- Line 508:
erroris assigned via walrus operator but never used (only checked for truthiness)- Line 540:
eis assigned but never usedThese can be simplified:
🔎 Proposed fix
- if error := raw_payload.get("error"): + if raw_payload.get("error"): MixinsUtils.handle_error(data)- except Exception as e: + except Exception: self.logger.exception("Sync failed")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
redocs/source/clients.rstredocs/source/conf.pyredocs/source/index.rstredocs/source/installation.rstredocs/source/quickstart.rstredocs/source/release_notes.rstsrc/pymax/files.pysrc/pymax/interfaces.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/pymax/interfaces.py
209-209: Do not catch blind exception: Exception
(BLE001)
251-251: Do not catch blind exception: Exception
(BLE001)
344-344: Redundant exception object included in logging.exception call
(TRY401)
362-362: Redundant exception object included in logging.exception call
(TRY401)
371-371: Redundant exception object included in logging.exception call
(TRY401)
386-386: Redundant exception object included in logging.exception call
(TRY401)
444-444: Do not catch blind exception: Exception
(BLE001)
469-472: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
508-508: Local variable error is assigned to but never used
(F841)
540-540: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (13)
redocs/source/installation.rst (1)
52-62: LGTM!The repository URLs have been consistently updated to
MaxApiTeam/PyMax.gitin both installation examples (pip and uv methods).src/pymax/files.py (1)
102-134:VideoandFiledo not support raw-only initialization.Unlike
Photo, bothVideo(lines 112-113) andFile(lines 131-132) raiseValueErrorif no path or url is provided, blocking raw-only initialization. This creates an inconsistent API wherePhotosupports raw bytes butVideo/Filedo not.If raw-only should be supported for all file types, consider adding a
nameparameter similar toPhoto:🔎 Proposed fix for Video
class Video(BaseFile): def __init__( - self, raw: bytes | None = None, *, url: str | None = None, path: str | None = None + self, raw: bytes | None = None, *, url: str | None = None, path: str | None = None, name: str | None = None ) -> None: self.file_name: str = "" if path: self.file_name = Path(path).name elif url: self.file_name = Path(url).name + elif name: + self.file_name = name + else: + self.file_name = "video" - if not self.file_name: + if not self.file_name and raw is None: raise ValueError("Either url or path must be provided.") super().__init__(raw=raw, url=url, path=path)src/pymax/interfaces.py (2)
35-108: LGTM!The
BaseClientclass provides well-structured lifecycle management, safe task execution, and proper cleanup logic. The async context manager implementation is correct.
200-211: Catching bareExceptionis intentional for ping resilience.The broad exception catch at line 209 with
exc_info=Truelogging is acceptable here since interactive ping failures shouldn't crash the connection loop.redocs/source/conf.py (1)
9-9: LGTM!The release version has been updated to
"1.2.4", resolving the previous version mismatch withpyproject.toml.redocs/source/index.rst (2)
38-49: LGTM!The toctree now includes release notes, and the new features list accurately reflects the v1.2.4 additions (profile photo upload, group resolution by link, contact support, contact list management).
106-117: LGTM!Repository references consistently updated to
MaxApiTeam/PyMaxfor both Star History and Contributors badges.redocs/source/quickstart.rst (2)
13-30: LGTM!Clear and helpful client selection guidance. The comparison between MaxClient (WebSocket/QR) and SocketMaxClient (TCP/phone) helps users choose the right client for their use case.
62-62: LGTM!The startup instruction correctly reflects the MaxClient flow which uses QR code authentication.
redocs/source/clients.rst (2)
4-31: LGTM!Excellent comparison table clearly showing the differences between MaxClient and SocketMaxClient. The feature matrix helps users make informed decisions about which client to use.
237-252: LGTM!Excellent workflow example demonstrating how to obtain a token via SocketMaxClient and reuse it with MaxClient for faster WebSocket connections.
redocs/source/release_notes.rst (2)
1-84: LGTM!Comprehensive release notes for v1.2.4 documenting:
- New raw bytes support in file classes
- Automatic read-receipt notifications
session_nameparameterget_current_web_version()utility- Enhanced User-Agent generation with
ua-generator- New
read_messagemethodThe changelog is well-organized and provides useful context for users upgrading.
88-143: LGTM!The v1.2.3 release notes section documents new features (
change_profile,resolve_group_by_link), new types (ContactAttach), and the newcontactsproperty.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
redocs/source/clients.rst (1)
103-109: Photo placeholder example could be more concrete.The
change_profile()example usesphoto=Photo(...)as a placeholder. While acceptable for documentation, consider providing a more explicit example of how to construct a Photo object (e.g., from file, from URL, etc.) to improve clarity for new users.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mdmkdocs.ymlpyproject.tomlredocs/source/clients.rst
💤 Files with no reviewable changes (1)
- mkdocs.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🔇 Additional comments (3)
redocs/source/clients.rst (3)
243-251: Token reuse workflow is well-documented.The example clearly shows how to extract a token from
SocketMaxClientand reuse it withMaxClientfor faster WebSocket connection, which is a valuable pattern for users.
103-109: Missing import for Photo in change_profile example.The example uses
Photo(...)but doesn't show where it's imported from. Add an import statement to clarify:from pymax.types import Photo(or the correct module path).
203-207: Verify completeness of SocketMaxClient initialization examples.The PR objectives mention a new
session_nameparameter. Consider adding this parameter to the SocketMaxClient examples (if it's a user-facing option) to ensure documentation completeness.Also applies to: 218-224
| <a href="https://github.com/MaxApiTeam/PyMax/graphs/contributors"> | ||
| <img src="https://contrib.rocks/image?repo=ink-developer/PyMax" /> |
There was a problem hiding this comment.
Inconsistent repository URL migration in contributors section.
Line 168 was updated to reference MaxApiTeam/PyMax, but the image source on Line 169 still points to the old organization (ink-developer/PyMax). This mismatch will cause the contributors badge to render stale data.
Additionally, Line 157 (Star History badge) also contains references to the old repository (ink-developer/PyMax appears twice in the URL) and may need similar updates for consistency.
🔎 Proposed fix to complete the repository migration
- <a href="https://github.com/MaxApiTeam/PyMax/graphs/contributors">
- <img src="https://contrib.rocks/image?repo=ink-developer/PyMax" />
+ <a href="https://github.com/MaxApiTeam/PyMax/graphs/contributors">
+ <img src="https://contrib.rocks/image?repo=MaxApiTeam/PyMax" />Also update Line 157:
-[](https://www.star-history.com/#ink-developer/PyMax&type=date&legend=top-left)
+[](https://www.star-history.com/#MaxApiTeam/PyMax&type=date&legend=top-left)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a href="https://github.com/MaxApiTeam/PyMax/graphs/contributors"> | |
| <img src="https://contrib.rocks/image?repo=ink-developer/PyMax" /> | |
| <a href="https://github.com/MaxApiTeam/PyMax/graphs/contributors"> | |
| <img src="https://contrib.rocks/image?repo=MaxApiTeam/PyMax" /> |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
169-169: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 Prompt for AI Agents
In README.md around lines 157 and 168-169, the contributors image and the Star
History badge still reference the old repository path ink-developer/PyMax while
other links use MaxApiTeam/PyMax; update the badge and image URLs to
consistently use MaxApiTeam/PyMax (replace all occurrences of
ink-developer/PyMax in those lines/URLs with MaxApiTeam/PyMax) so the
contributors image and star history reflect the migrated repository.
Описание
Кратко, что делает этот PR. Например, добавляет новый метод, исправляет баг, улучшает документацию.
Новые методы
Измененный методы
Новые параметры
Измененные типы
Да, методов мало но основная цель рефактор и улучшение безопастности
Тип изменений
Связанные задачи / Issue
Ссылка на issue, если есть: #
Тестирование
Покажите пример кода, который проверяет изменения:
Summary by CodeRabbit
New Features
Updated Types / Media
Documentation
Misc
✏️ Tip: You can customize this high-level summary in your review settings.