Messenger API for direct messaging without pub/sub#10
Conversation
- Add Messenger class for direct message sending through adapters - Add MessagingController with REST endpoints for direct messaging - Add send_to_tokens() method in APNHandler for direct APN notifications - Add send_to_emails() method in SMTPHandler with HTML support - Add send_to_urls() method in WebHandler with configurable HTTP methods - Add send_to_subscriptions() method in WebPushHandler for direct notifications - Add standalone handler classes for use without State owner - Refactor handlers to separate subscription resolution from sending logic - Handler methods now return result dictionaries with success status
📝 WalkthroughWalkthroughThis PR adds a Messenger facade and MessagingController for direct messaging (no pub/sub), refactors handlers to separate subscription resolution from delivery, and introduces direct-send methods ( Changes
Sequence DiagramsequenceDiagram
actor Client
participant Controller as MessagingController
participant Messenger
participant Handler as AdapterHandler
Client->>Controller: POST /messaging/apn (tokens, message, ...)
Controller->>Controller: validate payload & load app
Controller->>Messenger: instantiate with app/logger
Controller->>Messenger: send_apn(tokens, message, ...)
Messenger->>Handler: _StandaloneAPNHandler.send_to_tokens(tokens, message, ...)
Handler->>Handler: resolve creds, write temp files
Handler->>Handler: iterate tokens, send per-token, track sent set
Handler-->>Messenger: return { success, tokens: [...] }
Messenger-->>Controller: aggregate results
Controller-->>Client: HTTP response with result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 8
🤖 Fix all issues with AI agents
In @src/pushi/app/controllers/messaging.py:
- Around line 177-189: The call to messenger.send_email in messaging.py passes
an undefined keyword smtp_url which will raise a TypeError; either remove
smtp_url from the call site (in the block building result =
messenger.send_email(...)) if not used, or add smtp_url to the
Messenger.send_email method signature in src/pushi/base/messaging.py (and handle
it inside send_email) so the parameter is accepted; update any related tests or
callers accordingly.
- Around line 218-227: The controller calls Messenger.send_webhook with the
wrong parameter name: it passes url=... but send_webhook expects urls (plural).
Update the call in the controller (the block that creates pushi.Messenger and
calls send_webhook) to pass the correct argument name — e.g., change url=payload
to urls=[url] or urls=urls (depending on existing variable) so the call becomes
messenger.send_webhook(urls=[url], data=payload, headers=..., method=...);
ensure the passed value is a list of URLs if send_webhook expects an iterable.
- Around line 264-274: The controller calls Messenger.send_web_push with
individual endpoint/p256dh/auth args but the method expects a subscriptions
list; update the call in messaging.py where messenger = pushi.Messenger(...) and
result = messenger.send_web_push(...) to construct and pass
subscriptions=[{"endpoint": endpoint, "p256dh": p256dh, "auth": auth}]
(preserving vapid_private_key and vapid_email arguments) so the signature
matches send_web_push(subscriptions=...).
In @src/pushi/base/apn.py:
- Line 102: The method send_to_tokens currently uses a mutable default argument
invalid={} which can cause state to leak across calls; change the signature to
use invalid=None and at the start of send_to_tokens initialize it with a fresh
dict when None (e.g., if invalid is None: invalid = {}), ensuring any references
to invalid in the body still work unchanged while preventing shared state
between invocations.
In @src/pushi/base/messaging.py:
- Around line 121-123: The adapters normalization uses isinstance(adapters,
str), which breaks on Python 2 because unicode != str; update the type check to
a cross-version string test (e.g., use basestring on PY2 or six.string_types) so
both str and unicode are treated as strings; locate the adapters normalization
block (variable name "adapters" in messaging.py) and replace the isinstance
check accordingly, keeping the logic that wraps a single string into a list.
In @src/pushi/base/smtp.py:
- Line 184: Change the mutable default argument "invalid={}" to "invalid=None"
in the function signature and, at the start of the function body that uses the
"invalid" parameter, add "if invalid is None: invalid = {}" so each call gets a
fresh dict; update any other occurrences of the same pattern (e.g., similar code
in web.py) the same way to avoid shared state.
In @src/pushi/base/web_push.py:
- Line 164: Replace the mutable default argument invalid={} in the function
signature with invalid=None and inside the function (at the top) add a check
like "if invalid is None: invalid = {}" so that each call gets a fresh dict;
locate the parameter named invalid in the web push handler method (the function
containing invalid={} in src/pushi/base/web_push.py) and update both the
signature and initialisation accordingly.
In @src/pushi/base/web.py:
- Line 100: The send_to_urls method currently uses a mutable default argument
invalid={}, which can cause shared-state bugs; change the function signature to
use invalid=None and at the start of send_to_urls (function name: send_to_urls)
set invalid to an empty dict when it's None (use "if invalid is None: invalid =
{}") so each call gets a fresh dictionary instance.
🧹 Nitpick comments (4)
src/pushi/base/web.py (1)
188-189: Consider logging a warning for unsupported HTTP methods.Unknown HTTP methods are silently skipped. Adding a debug/warning log would help with troubleshooting when users accidentally specify invalid methods.
💡 Suggested improvement
else: + self.logger.warning("Unsupported HTTP method '%s', skipping" % method) continuesrc/pushi/base/smtp.py (1)
230-230: Coding guideline: prefer!= Noneoveris not None.As per coding guidelines, use
!= Nonecomparison style.💡 Suggested fix
- smtp_starttls = smtp_starttls if smtp_starttls is not None else smtp_config.get("starttls", False) + smtp_starttls = smtp_starttls if smtp_starttls != None else smtp_config.get("starttls", False)src/pushi/base/web_push.py (1)
314-319: Consider usinglogging.exceptionfor better error diagnostics.Using
self.logger.exception()instead ofself.logger.error()would include the full traceback, which aids debugging unexpected errors.💡 Suggested improvement
except Exception as exception: # logs any other unexpected errors - self.logger.error( + self.logger.exception( "Unexpected error sending Web Push to '%s': %s" % (endpoint, str(exception)) )src/pushi/base/messaging.py (1)
178-179: Consider narrowing the exception type.Catching blind
Exceptioncan mask unexpected errors. Consider catching more specific exceptions (e.g.,IOError,OSError,RuntimeError) or at minimum logging the exception type/traceback for debugging.Proposed improvement
except Exception as exception: - results[adapter] = dict(success=False, error=str(exception)) + self.logger.warning( + "Adapter '%s' failed: %s", adapter, str(exception) + ) + results[adapter] = dict(success=False, error=str(exception))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.mdsrc/pushi/app/controllers/__init__.pysrc/pushi/app/controllers/messaging.pysrc/pushi/base/__init__.pysrc/pushi/base/apn.pysrc/pushi/base/messaging.pysrc/pushi/base/smtp.pysrc/pushi/base/web.pysrc/pushi/base/web_push.py
🧰 Additional context used
📓 Path-based instructions (2)
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Always update
CHANGELOG.mdaccording to semantic versioning in the Unreleased section
Files:
CHANGELOG.md
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be formatted usingblackbefore committing
Python files must use CRLF as the line ending
Implementation must be compatible with Python 2.7+ and Python 3.13
No type annotations should exist in.pyfiles; use.pyifiles instead for type annotations
Use Python docstrings with:type:,:args:,:rtype:,:return:structure with a newline after the closing triple quotes
Preferitem not in listovernot item in list
Preferitem == Noneoveritem is None
Maintain consistent commenting style with the existing codebase
Do not commit code with debugging print statements or commented-out code
Files:
src/pushi/app/controllers/messaging.pysrc/pushi/app/controllers/__init__.pysrc/pushi/base/smtp.pysrc/pushi/base/web_push.pysrc/pushi/base/apn.pysrc/pushi/base/messaging.pysrc/pushi/base/__init__.pysrc/pushi/base/web.py
🧬 Code graph analysis (6)
src/pushi/app/controllers/messaging.py (1)
src/pushi/base/messaging.py (5)
send(66-181)send_apn(183-218)send_email(220-275)send_webhook(277-304)send_web_push(306-338)
src/pushi/app/controllers/__init__.py (1)
src/pushi/app/controllers/messaging.py (1)
MessagingController(36-275)
src/pushi/base/web_push.py (3)
src/pushi/base/messaging.py (4)
warning(390-391)debug(384-385)error(393-394)info(387-388)src/pushi/base/state.py (1)
get_channels(1101-1103)src/pushi/app/models/web_push.py (1)
WebPush(36-172)
src/pushi/base/apn.py (2)
src/pushi/app/models/base.py (3)
app_key(140-143)get(94-99)app_id(133-137)src/pushi/base/state.py (1)
get_channels(1101-1103)
src/pushi/base/messaging.py (3)
src/pushi/base/apn.py (4)
send(53-100)subscriptions(270-277)send_to_tokens(102-247)APNHandler(42-330)src/pushi/base/web.py (4)
send(57-98)subscriptions(223-230)send_to_urls(100-200)WebHandler(40-283)src/pushi/base/web_push.py (4)
send(68-155)subscriptions(376-395)send_to_subscriptions(157-321)WebPushHandler(51-506)
src/pushi/base/__init__.py (1)
src/pushi/base/messaging.py (1)
Messenger(39-338)
🪛 GitHub Actions: Main Workflow
src/pushi/base/smtp.py
[error] 1-1: Black formatting check failed. 4 files would be reformatted. Command: 'black . --check'.
src/pushi/base/web_push.py
[error] 1-1: Black formatting check failed. 4 files would be reformatted. Command: 'black . --check'.
src/pushi/base/apn.py
[error] 1-1: Black formatting check failed. 4 files would be reformatted. Command: 'black . --check'.
src/pushi/base/messaging.py
[error] 1-1: Black formatting check failed. 4 files would be reformatted. Command: 'black . --check'.
🪛 LanguageTool
CHANGELOG.md
[style] ~28-~28: ‘with success’ might be wordy. Consider a shorter alternative.
Context: ... methods now return result dictionaries with success status and sent targets ### Fixed * ...
(EN_WORDINESS_PREMIUM_WITH_SUCCESS)
🪛 Ruff (0.14.10)
src/pushi/base/smtp.py
184-184: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
src/pushi/base/web_push.py
122-122: Consider [event, *extra] instead of concatenation
Replace with [event, *extra]
(RUF005)
164-164: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
311-312: try-except-pass detected, consider logging the exception
(S110)
311-311: Do not catch blind exception: Exception
(BLE001)
314-314: Do not catch blind exception: Exception
(BLE001)
316-319: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/pushi/base/apn.py
78-78: Consider [event, *extra] instead of concatenation
Replace with [event, *extra]
(RUF005)
102-102: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
src/pushi/base/messaging.py
178-178: Do not catch blind exception: Exception
(BLE001)
src/pushi/base/web.py
100-100: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
146-146: Unused function argument: parser
(ARG001)
146-146: Unused function argument: message
(ARG001)
151-151: Unused function argument: protocol
(ARG001)
🔇 Additional comments (18)
CHANGELOG.md (1)
12-28: LGTM!The CHANGELOG entries correctly document the new Messenger API, MessagingController endpoints, and refactored handler methods in the Unreleased section following semantic versioning conventions.
src/pushi/base/__init__.py (1)
30-38: LGTM!The new imports correctly expose the
messagingmodule andMessengerclass at the package level, following the existing import pattern.src/pushi/app/controllers/__init__.py (1)
33-45: LGTM!The
MessagingControlleris properly registered following the existing controller import pattern.src/pushi/base/web.py (1)
97-98: Good refactoring pattern.The delegation from
send()tosend_to_urls()cleanly separates subscription resolution from the actual sending logic, enabling direct messaging without pub/sub.src/pushi/base/smtp.py (1)
168-169: Good refactoring pattern.The delegation from
send()tosend_to_emails()cleanly separates subscription resolution from the email sending logic, enabling direct messaging.src/pushi/base/web_push.py (1)
139-155: Good design for maintaining cleanup capability.Including
_idand_objin the subscription dict enables the direct-send path to properly clean up expired subscriptions, maintaining feature parity with the pub/sub path.src/pushi/base/apn.py (2)
66-100: Good refactoring pattern.The delegation from
send()tosend_to_tokens()cleanly separates subscription resolution from the APN sending logic. The token resolution correctly handles event aliases viaget_channels().
199-215: Solid temporary file cleanup implementation.The closure-based cleanup correctly tracks pending sends and removes temporary SSL key/certificate files only after all operations complete.
src/pushi/app/controllers/messaging.py (3)
36-43: LGTM!The controller class docstring is well-structured and clearly explains the purpose of the controller.
45-99: LGTM!The unified
sendendpoint correctly validates required parameters and delegates to the Messenger API. The input extraction and adapter configuration handling is clean.
101-140: LGTM!Proper validation of required
tokensandmessageparameters before delegation to the Messenger API.src/pushi/base/messaging.py (7)
39-64: LGTM!The Messenger class is well-designed with clear separation of concerns. The delegation to standalone handlers provides good code reuse while enabling stateless direct messaging.
183-218: LGTM!The
send_apnmethod is well-documented and correctly delegates to the handler with appropriate parameter mapping.
220-275: LGTM!The
send_emailmethod provides comprehensive SMTP configuration options with clear documentation.
277-304: LGTM!Clean delegation to the webhook handler with appropriate defaults.
306-338: LGTM!The
send_web_pushmethod correctly delegates to the handler with VAPID configuration support.
341-378: LGTM!The standalone handler classes are minimal and correctly initialize the required attributes (
owner,name,logger,subs) for stateless operation.
381-394: LGTM!The
_NullLoggerclass provides a clean no-op implementation for all standard log levels.
| result = messenger.send_email( | ||
| to=to, | ||
| subject=subject, | ||
| body=body, | ||
| smtp_host=data.get("smtp_host"), | ||
| smtp_port=data.get("smtp_port"), | ||
| smtp_user=data.get("smtp_user"), | ||
| smtp_password=data.get("smtp_password"), | ||
| smtp_starttls=data.get("smtp_starttls"), | ||
| smtp_sender=data.get("smtp_sender"), | ||
| smtp_url=data.get("smtp_url"), | ||
| html=data.get("html", False), | ||
| ) |
There was a problem hiding this comment.
Undeclared parameter smtp_url will cause a TypeError.
The Messenger.send_email method signature (from src/pushi/base/messaging.py lines 220-231) does not include an smtp_url parameter. Passing this keyword argument will raise a TypeError.
Proposed fix
Either remove the parameter if not needed:
result = messenger.send_email(
to=to,
subject=subject,
body=body,
smtp_host=data.get("smtp_host"),
smtp_port=data.get("smtp_port"),
smtp_user=data.get("smtp_user"),
smtp_password=data.get("smtp_password"),
smtp_starttls=data.get("smtp_starttls"),
smtp_sender=data.get("smtp_sender"),
- smtp_url=data.get("smtp_url"),
html=data.get("html", False),
)Or add smtp_url parameter support to Messenger.send_email if this functionality is intended.
📝 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.
| result = messenger.send_email( | |
| to=to, | |
| subject=subject, | |
| body=body, | |
| smtp_host=data.get("smtp_host"), | |
| smtp_port=data.get("smtp_port"), | |
| smtp_user=data.get("smtp_user"), | |
| smtp_password=data.get("smtp_password"), | |
| smtp_starttls=data.get("smtp_starttls"), | |
| smtp_sender=data.get("smtp_sender"), | |
| smtp_url=data.get("smtp_url"), | |
| html=data.get("html", False), | |
| ) | |
| result = messenger.send_email( | |
| to=to, | |
| subject=subject, | |
| body=body, | |
| smtp_host=data.get("smtp_host"), | |
| smtp_port=data.get("smtp_port"), | |
| smtp_user=data.get("smtp_user"), | |
| smtp_password=data.get("smtp_password"), | |
| smtp_starttls=data.get("smtp_starttls"), | |
| smtp_sender=data.get("smtp_sender"), | |
| html=data.get("html", False), | |
| ) |
🤖 Prompt for AI Agents
In @src/pushi/app/controllers/messaging.py around lines 177 - 189, The call to
messenger.send_email in messaging.py passes an undefined keyword smtp_url which
will raise a TypeError; either remove smtp_url from the call site (in the block
building result = messenger.send_email(...)) if not used, or add smtp_url to the
Messenger.send_email method signature in src/pushi/base/messaging.py (and handle
it inside send_email) so the parameter is accepted; update any related tests or
callers accordingly.
| # creates the messenger instance and sends the webhook | ||
| messenger = pushi.Messenger(logger=self.owner.logger) | ||
| result = messenger.send_webhook( | ||
| url=url, | ||
| data=payload, | ||
| headers=data.get("headers"), | ||
| method=data.get("method", "POST"), | ||
| ) | ||
|
|
||
| return result |
There was a problem hiding this comment.
Parameter name mismatch: url vs urls.
The Messenger.send_webhook method expects urls (plural) but the controller passes url (singular). This will raise a TypeError for unexpected keyword argument.
Proposed fix
# creates the messenger instance and sends the webhook
messenger = pushi.Messenger(logger=self.owner.logger)
result = messenger.send_webhook(
- url=url,
+ urls=url,
data=payload,
headers=data.get("headers"),
method=data.get("method", "POST"),
)🤖 Prompt for AI Agents
In @src/pushi/app/controllers/messaging.py around lines 218 - 227, The
controller calls Messenger.send_webhook with the wrong parameter name: it passes
url=... but send_webhook expects urls (plural). Update the call in the
controller (the block that creates pushi.Messenger and calls send_webhook) to
pass the correct argument name — e.g., change url=payload to urls=[url] or
urls=urls (depending on existing variable) so the call becomes
messenger.send_webhook(urls=[url], data=payload, headers=..., method=...);
ensure the passed value is a list of URLs if send_webhook expects an iterable.
src/pushi/base/apn.py
Outdated
| # delegates to the direct send method with resolved tokens | ||
| self.send_to_tokens(tokens, message, app=app, invalid=invalid) | ||
|
|
||
| def send_to_tokens(self, tokens, message, app=None, key_data=None, cer_data=None, sandbox=None, invalid={}): |
There was a problem hiding this comment.
Mutable default argument can cause unexpected state sharing.
Same issue as in other handlers - using invalid={} as a default argument can lead to state bleeding across calls.
🔧 Proposed fix
- def send_to_tokens(self, tokens, message, app=None, key_data=None, cer_data=None, sandbox=None, invalid={}):
+ def send_to_tokens(self, tokens, message, app=None, key_data=None, cer_data=None, sandbox=None, invalid=None):Then at the start of the method body:
if invalid == None:
invalid = {}🧰 Tools
🪛 Ruff (0.14.10)
102-102: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In @src/pushi/base/apn.py at line 102, The method send_to_tokens currently uses
a mutable default argument invalid={} which can cause state to leak across
calls; change the signature to use invalid=None and at the start of
send_to_tokens initialize it with a fresh dict when None (e.g., if invalid is
None: invalid = {}), ensuring any references to invalid in the body still work
unchanged while preventing shared state between invocations.
| # normalizes adapters to list for iteration | ||
| if isinstance(adapters, str): | ||
| adapters = [adapters] |
There was a problem hiding this comment.
Python 2.7 compatibility: isinstance(adapters, str) won't match unicode strings.
In Python 2, str and unicode are distinct types. Use basestring for Python 2/3 compatibility, or use a compatibility check.
Based on coding guidelines requiring Python 2.7+ and Python 3.13 compatibility.
Proposed fix
+import sys
+
+if sys.version_info[0] >= 3:
+ string_types = (str,)
+else:
+ string_types = (basestring,)
+
# ... in the send method:
# normalizes adapters to list for iteration
- if isinstance(adapters, str):
+ if isinstance(adapters, string_types):
adapters = [adapters]Alternatively, if the six library is already a dependency, use six.string_types.
🤖 Prompt for AI Agents
In @src/pushi/base/messaging.py around lines 121 - 123, The adapters
normalization uses isinstance(adapters, str), which breaks on Python 2 because
unicode != str; update the type check to a cross-version string test (e.g., use
basestring on PY2 or six.string_types) so both str and unicode are treated as
strings; locate the adapters normalization block (variable name "adapters" in
messaging.py) and replace the isinstance check accordingly, keeping the logic
that wraps a single string into a list.
| smtp_starttls=None, | ||
| smtp_sender=None, | ||
| html=False, | ||
| invalid={}, |
There was a problem hiding this comment.
Mutable default argument can cause unexpected state sharing.
Same issue as in web.py - using invalid={} as a default argument can lead to state bleeding across calls.
🔧 Proposed fix
- invalid={},
+ invalid=None,Then at the start of the method body:
if invalid == None:
invalid = {}🧰 Tools
🪛 Ruff (0.14.10)
184-184: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In @src/pushi/base/smtp.py at line 184, Change the mutable default argument
"invalid={}" to "invalid=None" in the function signature and, at the start of
the function body that uses the "invalid" parameter, add "if invalid is None:
invalid = {}" so each call gets a fresh dict; update any other occurrences of
the same pattern (e.g., similar code in web.py) the same way to avoid shared
state.
| app=None, | ||
| vapid_private_key=None, | ||
| vapid_email=None, | ||
| invalid={}, |
There was a problem hiding this comment.
Mutable default argument can cause unexpected state sharing.
Same issue as in other handlers - using invalid={} as a default argument can lead to state bleeding across calls.
🔧 Proposed fix
- invalid={},
+ invalid=None,Then at the start of the method body:
if invalid == None:
invalid = {}🧰 Tools
🪛 Ruff (0.14.10)
164-164: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In @src/pushi/base/web_push.py at line 164, Replace the mutable default argument
invalid={} in the function signature with invalid=None and inside the function
(at the top) add a check like "if invalid is None: invalid = {}" so that each
call gets a fresh dict; locate the parameter named invalid in the web push
handler method (the function containing invalid={} in
src/pushi/base/web_push.py) and update both the signature and initialisation
accordingly.
| # delegates to the direct send method with resolved URLs | ||
| self.send_to_urls(urls, json_d, invalid=invalid) | ||
|
|
||
| def send_to_urls(self, urls, data, headers=None, method="POST", invalid={}): |
There was a problem hiding this comment.
Mutable default argument can cause unexpected state sharing.
Using invalid={} as a default argument is a Python anti-pattern. The same dictionary instance is shared across all calls that don't provide the argument, which can lead to unexpected behavior.
🔧 Proposed fix
- def send_to_urls(self, urls, data, headers=None, method="POST", invalid={}):
+ def send_to_urls(self, urls, data, headers=None, method="POST", invalid=None):Then at the start of the method body:
if invalid == None:
invalid = {}🧰 Tools
🪛 Ruff (0.14.10)
100-100: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In @src/pushi/base/web.py at line 100, The send_to_urls method currently uses a
mutable default argument invalid={}, which can cause shared-state bugs; change
the function signature to use invalid=None and at the start of send_to_urls
(function name: send_to_urls) set invalid to an empty dict when it's None (use
"if invalid is None: invalid = {}") so each call gets a fresh dictionary
instance.
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new Messenger API for direct messaging through various adapters (APN, SMTP, Webhook, Web Push) without requiring pub/sub subscriptions. The PR refactors existing handler classes to separate subscription resolution from direct sending logic, enabling both subscription-based and direct messaging patterns.
Changes:
- Added new Messenger class and MessagingController with REST endpoints for direct messaging
- Refactored APNHandler, SMTPHandler, WebHandler, and WebPushHandler to extract direct sending methods
- Handler methods now return result dictionaries with success status and sent targets
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pushi/base/messaging.py | New Messenger API class with unified interface for direct messaging across adapters |
| src/pushi/app/controllers/messaging.py | New REST controller with endpoints for direct message sending |
| src/pushi/base/apn.py | Refactored to add send_to_tokens() method for direct APN notifications |
| src/pushi/base/smtp.py | Refactored to add send_to_emails() method with HTML support |
| src/pushi/base/web.py | Refactored to add send_to_urls() method with configurable HTTP methods |
| src/pushi/base/web_push.py | Refactored to add send_to_subscriptions() method for direct Web Push |
| src/pushi/base/init.py | Registered Messenger class for module exports |
| src/pushi/app/controllers/init.py | Registered MessagingController for module exports |
| CHANGELOG.md | Documented new features and refactoring changes |
Comments suppressed due to low confidence (1)
src/pushi/base/web.py:145
- Typo in comment: 'clojure' should be 'closure' when referring to a lexical closure in programming.
# creates the on message function that is going to be used at the end of
# the request to be able to close the protocol, this is a clojure and so
# current local variables will be exposed to the method
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
|
|
||
| # delegates to the direct send method with resolved emails | ||
| self.send_to_emails(emails, subject, body, app=app, invalid=invalid) |
There was a problem hiding this comment.
The send method in SMTPHandler now delegates to send_to_emails but does not return its result. This means the new result dictionary with success status will not be propagated to callers of the original send method. Consider adding return before the send_to_emails call for consistency with the new API design.
| self.send_to_emails(emails, subject, body, app=app, invalid=invalid) | |
| return self.send_to_emails(emails, subject, body, app=app, invalid=invalid) |
| # verifies if the pywebpush library is available | ||
| if not pywebpush: | ||
| self.logger.warning( | ||
| "pywebpush library not available, skipping Web Push notifications" | ||
| ) | ||
| return |
There was a problem hiding this comment.
Inconsistency in error handling: the original send method logs a warning and returns None when pywebpush is not available, but the new send_to_subscriptions method returns an error dictionary. This creates an inconsistency in the API where the same handler class returns different types depending on which method is called. Consider making the original send method also return a result dictionary for consistency.
| # retrieves the app from the current session | ||
| app_id = self.session.get("app_id", None) | ||
| app = pushi.App.get(ident=app_id) |
There was a problem hiding this comment.
The endpoint retrieves app from the session but does not validate that it exists before using it. If app_id is None or the app doesn't exist, this could lead to errors. Consider validating that the app exists and handle the case where it's not found with a proper error message.
| app_id = self.session.get("app_id", None) | ||
| app = pushi.App.get(ident=app_id) |
There was a problem hiding this comment.
The endpoint retrieves app from the session but does not validate that it exists before using it. If app_id is None or the app doesn't exist, this could lead to errors. Consider validating that the app exists and handle the case where it's not found with a proper error message.
| app_id = self.session.get("app_id", None) | |
| app = pushi.App.get(ident=app_id) | |
| app_id = self.session.get("app_id", None) | |
| if not app_id: | |
| raise appier.OperationalError(message="No app associated with current session") | |
| app = pushi.App.get(ident=app_id) | |
| if not app: | |
| raise appier.OperationalError(message="App not found") |
| ) | ||
|
|
||
|
|
||
| class _StandaloneAPNHandler(apn.APNHandler): |
There was a problem hiding this comment.
This class does not call APNHandler.init during initialization. (_StandaloneAPNHandler.init may be missing a call to a base class init)
| self.subs = {} | ||
|
|
||
|
|
||
| class _StandaloneSMTPHandler(smtp.SMTPHandler): |
There was a problem hiding this comment.
This class does not call SMTPHandler.init during initialization. (_StandaloneSMTPHandler.init may be missing a call to a base class init)
| self.subs = {} | ||
|
|
||
|
|
||
| class _StandaloneWebHandler(web.WebHandler): |
There was a problem hiding this comment.
This class does not call WebHandler.init during initialization. (_StandaloneWebHandler.init may be missing a call to a base class init)
| self.subs = {} | ||
|
|
||
|
|
||
| class _StandaloneWebPushHandler(web_push.WebPushHandler): |
There was a problem hiding this comment.
This class does not call WebPushHandler.init during initialization. (_StandaloneWebPushHandler.init may be missing a call to a base class init)
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as delete_exception: | |
| # ignore deletion failures but log for observability | |
| self.logger.warning( | |
| "Failed to delete expired subscription '%s': %s" | |
| % (endpoint, str(delete_exception)) | |
| ) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ting - Added validation for application identifier presence in MessagingController. - Included error handling for cases where the application is not found. - Refactored comments and code formatting in various files for better readability. - Updated submodule reference in js directory to indicate a dirty state.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/pushi/app/controllers/messaging.py:
- Around line 118-145: The endpoints calling pushi.Messenger (send_apn,
send_email, send_web_push) currently proceed when app_id/app is missing,
allowing Messenger to be called with app=None; add the same app validation used
in send(): retrieve app_id from session, load app via
pushi.App.get(ident=app_id), and if app is None raise
appier.OperationalError("No app selected" or "App not found"); factor this into
a helper (e.g., _get_current_app or ensure_app) and reuse it from the methods
that call pushi.Messenger (the blocks around send_apn, send_email,
send_web_push) before constructing Messenger so no call is made with app=None.
- Around line 167-178: The send_email handler currently rejects falsy bodies
using "if not body" which disallows empty-string bodies; change the validation
to only reject missing bodies by testing "if body is None" (i.e., replace the
falsy check on the variable body with an explicit None check) so that
empty-string bodies are accepted; keep the same appier.OperationalError raised
when the value is None and update the body check near the parameter extraction
(variables to, subject, body) in the send_email function.
🧹 Nitpick comments (1)
src/pushi/app/controllers/messaging.py (1)
67-104: Good:/messaging/sendvalidates session app context; consider de-duplicating app lookup.
You repeat the same “getapp_idfrom session → load app → error if missing” pattern in other handlers; extracting a small helper will reduce divergence bugs (and there already is divergence in other methods).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pushi/app/controllers/messaging.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be formatted usingblackbefore committing
Python files must use CRLF as the line ending
Implementation must be compatible with Python 2.7+ and Python 3.13
No type annotations should exist in.pyfiles; use.pyifiles instead for type annotations
Use Python docstrings with:type:,:args:,:rtype:,:return:structure with a newline after the closing triple quotes
Preferitem not in listovernot item in list
Preferitem == Noneoveritem is None
Maintain consistent commenting style with the existing codebase
Do not commit code with debugging print statements or commented-out code
Files:
src/pushi/app/controllers/messaging.py
🧬 Code graph analysis (1)
src/pushi/app/controllers/messaging.py (6)
src/pushi/base/web.py (1)
send(57-98)src/pushi/base/apn.py (1)
send(53-100)src/pushi/base/smtp.py (1)
send(79-169)src/pushi/base/web_push.py (1)
send(68-155)src/pushi/app/models/base.py (2)
app_id(133-137)get(94-99)src/pushi/app/models/app.py (1)
App(39-269)
🔇 Additional comments (2)
src/pushi/app/controllers/messaging.py (2)
252-277: API signature verification required: confirmpushi.Messenger.send_web_pushparameter contract.The concern about parameter mismatch (endpoint/p256dh/auth vs. subscriptions) cannot be verified without access to the Messenger class implementation. Please verify the actual method signature and confirm whether it expects individual parameters (endpoint, p256dh, auth) or a subscriptions payload object to ensure this endpoint will not error at runtime.
210-231: Fix guideline violation: usepayload == Noneinstead ofpayload is None.Per coding guidelines, use
item == Noneoveritem is None. Additionally, consider hardening the webhook endpoint against SSRF by: restricting HTTP methods to an allowlist (e.g., POST, PUT, PATCH, DELETE) and validating that the URL scheme ishttporhttps. Manual code review of thesend_webhookimplementation is recommended to assess the security risk level and confirm what validation is already in place.
| class MessagingController(appier.Controller): | ||
| """ | ||
| Controller for direct message sending through various adapters | ||
| without requiring pub/sub subscriptions. | ||
|
|
||
| Provides HTTP endpoints for sending messages via APN, SMTP, | ||
| Webhook, and Web Push adapters using the Messenger API. | ||
| """ | ||
|
|
||
| @appier.private | ||
| @appier.route("/messaging/send", "POST") | ||
| def send(self): | ||
| """ | ||
| Sends a message through one or more adapters. | ||
|
|
||
| Accepts a JSON payload with adapter configuration and targets, | ||
| allowing the same message to be sent through multiple channels. | ||
|
|
||
| Example payload: | ||
| { | ||
| "adapters": ["apn", "email", "web_push"], | ||
| "data": {"title": "Hello", "body": "World"}, | ||
| "apn_tokens": ["device_token_1"], | ||
| "email_to": ["user@example.com"], | ||
| "email_subject": "Notification", | ||
| "web_push_subscriptions": [ | ||
| {"endpoint": "...", "p256dh": "...", "auth": "..."} | ||
| ] | ||
| } | ||
| """ |
There was a problem hiding this comment.
Docstrings don’t match project-required format (:type:/:args:/:rtype:/:return:).
This will likely fail repo conventions for **/*.py docstrings. Consider converting each method docstring to the required structured format (you can keep the JSON examples under :args:/notes).
Also applies to: 108-116, 149-158, 200-208, 236-246
| # extracts required parameters | ||
| to = data.get("to") | ||
| subject = data.get("subject") | ||
| body = data.get("body") | ||
|
|
||
| if not to: | ||
| raise appier.OperationalError(message="No recipient specified") | ||
| if not subject: | ||
| raise appier.OperationalError(message="No subject specified") | ||
| if not body: | ||
| raise appier.OperationalError(message="No body specified") | ||
|
|
There was a problem hiding this comment.
send_email: body check rejects empty-string bodies; consider body == None instead of not body.
If empty body is acceptable, change the validation accordingly.
Possible adjustment
- if not body:
+ if body == None:
raise appier.OperationalError(message="No body specified")📝 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.
| # extracts required parameters | |
| to = data.get("to") | |
| subject = data.get("subject") | |
| body = data.get("body") | |
| if not to: | |
| raise appier.OperationalError(message="No recipient specified") | |
| if not subject: | |
| raise appier.OperationalError(message="No subject specified") | |
| if not body: | |
| raise appier.OperationalError(message="No body specified") | |
| # extracts required parameters | |
| to = data.get("to") | |
| subject = data.get("subject") | |
| body = data.get("body") | |
| if not to: | |
| raise appier.OperationalError(message="No recipient specified") | |
| if not subject: | |
| raise appier.OperationalError(message="No subject specified") | |
| if body == None: | |
| raise appier.OperationalError(message="No body specified") |
🤖 Prompt for AI Agents
In @src/pushi/app/controllers/messaging.py around lines 167 - 178, The
send_email handler currently rejects falsy bodies using "if not body" which
disallows empty-string bodies; change the validation to only reject missing
bodies by testing "if body is None" (i.e., replace the falsy check on the
variable body with an explicit None check) so that empty-string bodies are
accepted; keep the same appier.OperationalError raised when the value is None
and update the body check near the parameter extraction (variables to, subject,
body) in the send_email function.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @src/pushi/app/controllers/messaging.py:
- Around line 190-206: The call site passes an unsupported smtp_url kwarg to
Messenger.send_email which will raise a TypeError; either remove smtp_url from
the call in the controller (where messenger = pushi.Messenger(...) and result =
messenger.send_email(...)) or add smtp_url as a parameter to
Messenger.send_email in the Messenger class (update signature in send_email and
any internal handling/forwarding of SMTP connection details). Ensure parameter
names stay consistent between the controller call and the send_email definition
and handle smtp_url appropriately (e.g., parse/use it or fall back to individual
smtp_host/port/user/password fields).
- Around line 284-294: The controller is calling Messenger.send_web_push(...)
with endpoint, p256dh, auth as separate arguments but send_web_push expects a
subscriptions list; fix by building a subscription dict like {"endpoint":
endpoint, "keys": {"p256dh": p256dh, "auth": auth}} and pass it as
subscriptions=[subscription] to messenger.send_web_push (keep message,
vapid_private_key and vapid_email arguments), and remove the individual
endpoint/p256dh/auth keyword arguments so the call matches the send_web_push
signature.
In @src/pushi/base/apn.py:
- Around line 102-111: The send_to_tokens function currently uses a mutable
default argument invalid={} which can leak state across calls; change the
signature to use invalid=None and inside send_to_tokens initialize invalid = {}
if invalid is None (and preserve any existing behavior where a dict passed in is
used), update any type hints or callers if needed, and ensure subsequent
uses/reference to the invalid variable remain the same.
In @src/pushi/base/messaging.py:
- Around line 274-301: The send_webhook method currently defines parameter
`urls` but callers pass `url`, causing a TypeError; update send_webhook in
class/method send_webhook to accept an optional `url=None` argument (or accept
**kwargs) and normalize it to `urls` before calling `_web_handler.send_to_urls`
(e.g., if url is provided and urls is None, set urls = url or [url] as
appropriate), and update the docstring to mention both `url` and `urls` as
accepted keywords so the controller call `messenger.send_webhook(url=...)` works
without error.
In @src/pushi/base/smtp.py:
- Around line 171-185: The send_to_emails method uses a mutable default argument
invalid={} which can persist state across calls; change the signature to use
invalid=None and inside send_to_emails set invalid = {} if invalid is None, then
proceed to use that local dict; update any internal references to the invalid
variable accordingly and check/adjust any call sites that relied on passing
nothing so behavior remains the same.
🧹 Nitpick comments (3)
src/pushi/base/apn.py (1)
141-145: Use== Noneinstead ofis Noneper coding guidelines.The project coding guidelines specify preferring
item == Noneoveritem is None.♻️ Proposed fix
- if sandbox is None: + if sandbox == None: sandbox = getattr(app, "apn_sandbox", False) - if sandbox is None: + if sandbox == None: sandbox = Falsesrc/pushi/base/web_push.py (1)
309-316: Silent exception swallowing during subscription deletion loses diagnostic info.When a 410/404 indicates an expired subscription, the deletion failure is silently ignored. Consider logging at debug level to aid troubleshooting.
♻️ Proposed fix
sub_obj = sub.get("_obj") if sub_obj: try: sub_obj.delete() - except Exception: - pass + except Exception as delete_error: + self.logger.debug( + "Failed to delete expired subscription: %s" + % str(delete_error) + )src/pushi/app/controllers/messaging.py (1)
234-241: Webhook sends single URL butsend_webhookexpectsurlsparameter.The controller extracts
url(singular) butMessenger.send_webhook()expectsurls(plural/list). The handler normalizes strings to lists, so this works, but consider using consistent naming in the controller.♻️ Suggested improvement for clarity
- result = messenger.send_webhook( - url=url, + result = messenger.send_webhook( + urls=url, data=payload, headers=data.get("headers"), method=data.get("method", "POST"), )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/pushi/app/controllers/messaging.pysrc/pushi/base/apn.pysrc/pushi/base/messaging.pysrc/pushi/base/smtp.pysrc/pushi/base/web_push.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must be formatted usingblackbefore committing
Python files must use CRLF as the line ending
Implementation must be compatible with Python 2.7+ and Python 3.13
No type annotations should exist in.pyfiles; use.pyifiles instead for type annotations
Use Python docstrings with:type:,:args:,:rtype:,:return:structure with a newline after the closing triple quotes
Preferitem not in listovernot item in list
Preferitem == Noneoveritem is None
Maintain consistent commenting style with the existing codebase
Do not commit code with debugging print statements or commented-out code
Files:
src/pushi/base/smtp.pysrc/pushi/base/web_push.pysrc/pushi/app/controllers/messaging.pysrc/pushi/base/messaging.pysrc/pushi/base/apn.py
🧠 Learnings (2)
📚 Learning: 2026-01-08T23:11:04.733Z
Learnt from: CR
Repo: hivesolutions/pushi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T23:11:04.733Z
Learning: Applies to **/*.py : Use Python docstrings with `:type:`, `:args:`, `:rtype:`, `:return:` structure with a newline after the closing triple quotes
Applied to files:
src/pushi/app/controllers/messaging.py
📚 Learning: 2026-01-08T23:11:04.733Z
Learnt from: CR
Repo: hivesolutions/pushi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T23:11:04.733Z
Learning: Applies to **/*.py : Maintain consistent commenting style with the existing codebase
Applied to files:
src/pushi/app/controllers/messaging.py
🧬 Code graph analysis (3)
src/pushi/base/web_push.py (4)
src/pushi/base/messaging.py (4)
warning(387-388)debug(381-382)error(390-391)info(384-385)src/pushi/app/models/base.py (3)
app_key(140-143)get(94-99)app_id(133-137)src/pushi/base/state.py (1)
get_channels(1101-1103)src/pushi/app/models/web_push.py (1)
WebPush(36-172)
src/pushi/app/controllers/messaging.py (4)
src/pushi/base/apn.py (1)
send(53-100)src/pushi/base/smtp.py (1)
send(79-169)src/pushi/base/web_push.py (1)
send(68-159)src/pushi/base/web.py (1)
send(57-98)
src/pushi/base/apn.py (2)
src/pushi/app/models/base.py (3)
app_key(140-143)get(94-99)app_id(133-137)src/pushi/base/state.py (1)
get_channels(1101-1103)
🪛 Ruff (0.14.10)
src/pushi/base/smtp.py
184-184: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
src/pushi/base/web_push.py
122-122: Consider [event, *extra] instead of concatenation
Replace with [event, *extra]
(RUF005)
168-168: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
315-316: try-except-pass detected, consider logging the exception
(S110)
315-315: Do not catch blind exception: Exception
(BLE001)
318-318: Do not catch blind exception: Exception
(BLE001)
320-323: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/pushi/base/messaging.py
175-175: Do not catch blind exception: Exception
(BLE001)
src/pushi/base/apn.py
78-78: Consider [event, *extra] instead of concatenation
Replace with [event, *extra]
(RUF005)
110-110: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
150-150: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (19)
src/pushi/base/apn.py (3)
66-100: LGTM - Clean refactoring ofsend()to delegate tosend_to_tokens().The subscription resolution logic is properly extracted, tokens are deduplicated using a set, and the delegation pattern enables direct-send use cases while maintaining the pub/sub flow.
156-163: Token normalization and early return look correct.The handling of single string tokens and empty token sets is appropriate. Early return with an empty result dictionary is a clean pattern.
198-256: LGTM - Core APN sending logic is well-structured.The cleanup closure pattern, temporary file management, and per-token iteration with invalid tracking are correctly implemented. The result dictionary with sent tokens provides good observability.
src/pushi/base/web_push.py (3)
90-159: LGTM -send()correctly orchestrates batch fetch and delegation.The subscription ID aggregation, batch database fetch with
$in, and construction of subscription dicts with_objreferences for cleanup are well-designed. The delegation tosend_to_subscriptionsenables the direct-send use case.
193-238: LGTM - VAPID credential resolution and early returns are correct.The fallback chain (direct params → app → defaults), mailto prefix normalization, and PEM-to-base64url conversion are properly implemented.
256-325: LGTM - Per-subscription iteration with error handling.The validation of required fields, invalid-map tracking, and per-subscription exception handling provide resilience. Returning the sent endpoints list enables observability.
src/pushi/base/smtp.py (4)
139-169: LGTM - Clean delegation fromsend()tosend_to_emails().The subject/body construction logic remains in
send()appropriately, and the delegation tosend_to_emails()enables the direct-send use case.
220-307: LGTM - SMTP configuration resolution and sending logic.The priority chain (direct params → app smtp_url → global SMTP_URL → individual env vars), validation of required config, MIME type handling for HTML, and per-recipient iteration are well-implemented.
397-431: LGTM -_resolve_smtp_confighelper is well-structured.Clean fallback chain implementation with proper URL parsing and individual env var fallback.
434-483: LGTM -parse_smtp_urlhelper handles edge cases well.Proper handling of smtps scheme, default ports, URL-encoded credentials, and query string parsing for sender.
src/pushi/app/controllers/messaging.py (3)
208-242: Webhook endpoint doesn't require app authentication unlike other endpoints.The
/messaging/webhookendpoint doesn't validateapp_idfrom the session, while all other endpoints do. This may be intentional (webhooks don't need app credentials), but could also be a security gap if webhook configuration should be app-scoped.Please confirm this is intentional. If webhooks should be app-scoped, add the same app validation pattern used in other endpoints.
45-104: LGTM - Multi-adapter/messaging/sendendpoint is well-structured.The validation of app context, extraction of adapter-specific parameters, and delegation to
Messenger.send()are correctly implemented.
106-150: LGTM - APN endpoint validates inputs and delegates correctly.Token and message validation, app context verification, and proper delegation to
messenger.send_apn().src/pushi/base/messaging.py (6)
39-64: LGTM - Messenger facade design is clean.The facade pattern with lightweight standalone handlers enables direct messaging without requiring a State owner. The
_NullLoggerfallback prevents null pointer issues when no logger is provided.
127-178: Broad exception handling is appropriate here for adapter resilience.Catching
Exceptionat line 175 ensures one adapter failure doesn't prevent other adapters from being tried. The error is captured in the results dictionary for observability.
180-215: LGTM -send_apn()delegation is correct.Proper delegation to the standalone APN handler with app context passed through.
217-272: LGTM -send_email()delegation is correct.All SMTP parameters are properly forwarded to the handler.
303-335: LGTM -send_web_push()correctly expects subscriptions list.The method correctly expects a list of subscription dictionaries. The controller issue (passing individual fields) was already flagged.
338-391: LGTM - Standalone handlers correctly bypass parent__init__.The standalone handlers set required attributes directly without calling the parent
__init__, which would require an owner. The_NullLoggerprovides a complete no-op interface.
| # creates the messenger instance and sends the email | ||
| messenger = pushi.Messenger(app=app, logger=self.owner.logger) | ||
| result = messenger.send_email( | ||
| to=to, | ||
| subject=subject, | ||
| body=body, | ||
| smtp_host=data.get("smtp_host"), | ||
| smtp_port=data.get("smtp_port"), | ||
| smtp_user=data.get("smtp_user"), | ||
| smtp_password=data.get("smtp_password"), | ||
| smtp_starttls=data.get("smtp_starttls"), | ||
| smtp_sender=data.get("smtp_sender"), | ||
| smtp_url=data.get("smtp_url"), | ||
| html=data.get("html", False), | ||
| ) | ||
|
|
||
| return result |
There was a problem hiding this comment.
Parameter smtp_url is passed but not accepted by Messenger.send_email().
Looking at the Messenger.send_email() signature in src/pushi/base/messaging.py (lines 217-272), it doesn't accept an smtp_url parameter. This will cause a TypeError at runtime.
🐛 Proposed fix - Remove the parameter or add support in Messenger
Option 1: Remove the parameter if SMTP URL isn't needed for direct API calls:
result = messenger.send_email(
to=to,
subject=subject,
body=body,
smtp_host=data.get("smtp_host"),
smtp_port=data.get("smtp_port"),
smtp_user=data.get("smtp_user"),
smtp_password=data.get("smtp_password"),
smtp_starttls=data.get("smtp_starttls"),
smtp_sender=data.get("smtp_sender"),
- smtp_url=data.get("smtp_url"),
html=data.get("html", False),
)Option 2: Add smtp_url support to Messenger.send_email() in src/pushi/base/messaging.py.
📝 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.
| # creates the messenger instance and sends the email | |
| messenger = pushi.Messenger(app=app, logger=self.owner.logger) | |
| result = messenger.send_email( | |
| to=to, | |
| subject=subject, | |
| body=body, | |
| smtp_host=data.get("smtp_host"), | |
| smtp_port=data.get("smtp_port"), | |
| smtp_user=data.get("smtp_user"), | |
| smtp_password=data.get("smtp_password"), | |
| smtp_starttls=data.get("smtp_starttls"), | |
| smtp_sender=data.get("smtp_sender"), | |
| smtp_url=data.get("smtp_url"), | |
| html=data.get("html", False), | |
| ) | |
| return result | |
| # creates the messenger instance and sends the email | |
| messenger = pushi.Messenger(app=app, logger=self.owner.logger) | |
| result = messenger.send_email( | |
| to=to, | |
| subject=subject, | |
| body=body, | |
| smtp_host=data.get("smtp_host"), | |
| smtp_port=data.get("smtp_port"), | |
| smtp_user=data.get("smtp_user"), | |
| smtp_password=data.get("smtp_password"), | |
| smtp_starttls=data.get("smtp_starttls"), | |
| smtp_sender=data.get("smtp_sender"), | |
| html=data.get("html", False), | |
| ) | |
| return result |
🤖 Prompt for AI Agents
In @src/pushi/app/controllers/messaging.py around lines 190 - 206, The call site
passes an unsupported smtp_url kwarg to Messenger.send_email which will raise a
TypeError; either remove smtp_url from the call in the controller (where
messenger = pushi.Messenger(...) and result = messenger.send_email(...)) or add
smtp_url as a parameter to Messenger.send_email in the Messenger class (update
signature in send_email and any internal handling/forwarding of SMTP connection
details). Ensure parameter names stay consistent between the controller call and
the send_email definition and handle smtp_url appropriately (e.g., parse/use it
or fall back to individual smtp_host/port/user/password fields).
| # creates the messenger instance and sends the notification | ||
| messenger = pushi.Messenger(app=app, logger=self.owner.logger) | ||
| result = messenger.send_web_push( | ||
| endpoint=endpoint, | ||
| p256dh=p256dh, | ||
| auth=auth, | ||
| message=message, | ||
| vapid_private_key=data.get("vapid_private_key"), | ||
| vapid_email=data.get("vapid_email"), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Signature mismatch: send_web_push() expects subscriptions list, not individual fields.
The Messenger.send_web_push() method expects a subscriptions parameter containing a list of subscription dictionaries. The controller is passing endpoint, p256dh, auth as individual parameters, which don't match the method signature and will cause a TypeError.
🐛 Proposed fix
# creates the messenger instance and sends the notification
messenger = pushi.Messenger(app=app, logger=self.owner.logger)
result = messenger.send_web_push(
- endpoint=endpoint,
- p256dh=p256dh,
- auth=auth,
- message=message,
+ subscriptions=[{
+ "endpoint": endpoint,
+ "p256dh": p256dh,
+ "auth": auth,
+ }],
+ message=message or data.get("data"),
vapid_private_key=data.get("vapid_private_key"),
vapid_email=data.get("vapid_email"),
)🤖 Prompt for AI Agents
In @src/pushi/app/controllers/messaging.py around lines 284 - 294, The
controller is calling Messenger.send_web_push(...) with endpoint, p256dh, auth
as separate arguments but send_web_push expects a subscriptions list; fix by
building a subscription dict like {"endpoint": endpoint, "keys": {"p256dh":
p256dh, "auth": auth}} and pass it as subscriptions=[subscription] to
messenger.send_web_push (keep message, vapid_private_key and vapid_email
arguments), and remove the individual endpoint/p256dh/auth keyword arguments so
the call matches the send_web_push signature.
| def send_to_tokens( | ||
| self, | ||
| tokens, | ||
| message, | ||
| app=None, | ||
| key_data=None, | ||
| cer_data=None, | ||
| sandbox=None, | ||
| invalid={}, | ||
| ): |
There was a problem hiding this comment.
Mutable default argument invalid={} can cause unexpected behavior.
Using a mutable default argument means the same dictionary instance is shared across all calls that don't provide an explicit value. This can lead to subtle bugs where the invalid dict accumulates entries from previous invocations.
🐛 Proposed fix
def send_to_tokens(
self,
tokens,
message,
app=None,
key_data=None,
cer_data=None,
sandbox=None,
- invalid={},
+ invalid=None,
):
+ if invalid == None:
+ invalid = {}🧰 Tools
🪛 Ruff (0.14.10)
110-110: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In @src/pushi/base/apn.py around lines 102 - 111, The send_to_tokens function
currently uses a mutable default argument invalid={} which can leak state across
calls; change the signature to use invalid=None and inside send_to_tokens
initialize invalid = {} if invalid is None (and preserve any existing behavior
where a dict passed in is used), update any type hints or callers if needed, and
ensure subsequent uses/reference to the invalid variable remain the same.
| def send_webhook( | ||
| self, | ||
| urls, | ||
| data, | ||
| headers=None, | ||
| method="POST", | ||
| ): | ||
| """ | ||
| Sends HTTP requests to one or more webhook endpoints. | ||
|
|
||
| :type urls: String/List | ||
| :param urls: Webhook URL(s) to send the request to. | ||
| :type data: Dictionary/String | ||
| :param data: Data to send (dictionaries are JSON-encoded). | ||
| :type headers: Dictionary | ||
| :param headers: Additional headers to include. | ||
| :type method: String | ||
| :param method: HTTP method to use (default: POST). | ||
| :rtype: Dictionary | ||
| :return: Result with success status and sent URLs. | ||
| """ | ||
|
|
||
| return self._web_handler.send_to_urls( | ||
| urls=urls, | ||
| data=data, | ||
| headers=headers, | ||
| method=method, | ||
| ) |
There was a problem hiding this comment.
send_webhook() accepts urls but controller passes url as keyword.
The controller at line 236 calls messenger.send_webhook(url=url, ...) but this method's signature uses urls. This will cause a TypeError at runtime.
🐛 Proposed fix in controller (already noted above)
In src/pushi/app/controllers/messaging.py, change line 236:
- url=url,
+ urls=url,🤖 Prompt for AI Agents
In @src/pushi/base/messaging.py around lines 274 - 301, The send_webhook method
currently defines parameter `urls` but callers pass `url`, causing a TypeError;
update send_webhook in class/method send_webhook to accept an optional
`url=None` argument (or accept **kwargs) and normalize it to `urls` before
calling `_web_handler.send_to_urls` (e.g., if url is provided and urls is None,
set urls = url or [url] as appropriate), and update the docstring to mention
both `url` and `urls` as accepted keywords so the controller call
`messenger.send_webhook(url=...)` works without error.
| def send_to_emails( | ||
| self, | ||
| emails, | ||
| subject, | ||
| body, | ||
| app=None, | ||
| smtp_host=None, | ||
| smtp_port=None, | ||
| smtp_user=None, | ||
| smtp_password=None, | ||
| smtp_starttls=None, | ||
| smtp_sender=None, | ||
| html=False, | ||
| invalid={}, | ||
| ): |
There was a problem hiding this comment.
Mutable default argument invalid={} can cause unexpected behavior.
Same issue as in other handlers - the shared dictionary instance persists across calls.
🐛 Proposed fix
def send_to_emails(
self,
emails,
subject,
body,
app=None,
smtp_host=None,
smtp_port=None,
smtp_user=None,
smtp_password=None,
smtp_starttls=None,
smtp_sender=None,
html=False,
- invalid={},
+ invalid=None,
):
+ if invalid == None:
+ invalid = {}📝 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.
| def send_to_emails( | |
| self, | |
| emails, | |
| subject, | |
| body, | |
| app=None, | |
| smtp_host=None, | |
| smtp_port=None, | |
| smtp_user=None, | |
| smtp_password=None, | |
| smtp_starttls=None, | |
| smtp_sender=None, | |
| html=False, | |
| invalid={}, | |
| ): | |
| def send_to_emails( | |
| self, | |
| emails, | |
| subject, | |
| body, | |
| app=None, | |
| smtp_host=None, | |
| smtp_port=None, | |
| smtp_user=None, | |
| smtp_password=None, | |
| smtp_starttls=None, | |
| smtp_sender=None, | |
| html=False, | |
| invalid=None, | |
| ): | |
| if invalid == None: | |
| invalid = {} |
🧰 Tools
🪛 Ruff (0.14.10)
184-184: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In @src/pushi/base/smtp.py around lines 171 - 185, The send_to_emails method
uses a mutable default argument invalid={} which can persist state across calls;
change the signature to use invalid=None and inside send_to_emails set invalid =
{} if invalid is None, then proceed to use that local dict; update any internal
references to the invalid variable accordingly and check/adjust any call sites
that relied on passing nothing so behavior remains the same.
| def send_to_subscriptions( | ||
| self, | ||
| subscriptions, | ||
| message, | ||
| app=None, | ||
| vapid_private_key=None, | ||
| vapid_email=None, | ||
| invalid={}, | ||
| ): |
There was a problem hiding this comment.
Mutable default argument invalid={} can cause unexpected behavior.
Same issue as in apn.py - the shared dictionary instance persists across calls.
🐛 Proposed fix
def send_to_subscriptions(
self,
subscriptions,
message,
app=None,
vapid_private_key=None,
vapid_email=None,
- invalid={},
+ invalid=None,
):
+ if invalid == None:
+ invalid = {}📝 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.
| def send_to_subscriptions( | |
| self, | |
| subscriptions, | |
| message, | |
| app=None, | |
| vapid_private_key=None, | |
| vapid_email=None, | |
| invalid={}, | |
| ): | |
| def send_to_subscriptions( | |
| self, | |
| subscriptions, | |
| message, | |
| app=None, | |
| vapid_private_key=None, | |
| vapid_email=None, | |
| invalid=None, | |
| ): | |
| if invalid == None: | |
| invalid = {} |
🧰 Tools
🪛 Ruff (0.14.10)
168-168: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
This pull request introduces a new Messenger API for direct messaging, along with a
MessagingControllerthat exposes REST endpoints for sending messages through various adapters (APN, email, webhook, web push) without requiring pub/sub subscriptions. It also refactors the handler classes to separate subscription resolution from direct sending logic and adds methods for direct message delivery to specific targets. Handler methods now return result dictionaries with success status and details of sent targets.New Features:
MessagingControllerwith REST endpoints for sending messages via/messaging/send,/messaging/apn,/messaging/email,/messaging/webhook, and/messaging/web_push. [1] [2]send_to_tokens()inAPNHandler,send_to_emails()inSMTPHandler,send_to_urls()inWebHandler, andsend_to_subscriptions()inWebPushHandler.Refactoring and Internal Improvements:
APNHandler,SMTPHandler,WebHandler,WebPushHandler) to separate subscription resolution from direct sending logic, improving code modularity and reusability. [1] [2] [3]Codebase Integration:
__init__.pyfiles for proper module exposure. [1] [2] [3]These changes collectively provide a flexible, unified API for direct messaging through multiple channels, and improve the maintainability and clarity of the codebase.
Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.