feat: add Web Push support and enhance App model with VAPID fields #4
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds W3C Web Push support: new WebPush ORM and controller endpoints, WebPushHandler with VAPID/pywebpush support and in-memory subscription management, App model VAPID fields, removal of legacy PushAPIHandler, state integration, tests, CI and dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as REST Client
participant Controller as WebPushController
participant Handler as WebPushHandler
participant DB as Database (WebPush / App)
participant PyWP as pywebpush
Note over Client,Controller: Subscribe flow
Client->>Controller: POST /web_pushes (payload)
Controller->>Handler: subscribe(web_push, auth, unsubscribe)
Handler->>DB: create/query subscription record
DB-->>Handler: stored subscription
Handler->>Handler: update in-memory map
Handler-->>Controller: serialized subscription
Controller-->>Client: 200 OK
Note over Client,Handler: Send notification
Client->>Handler: send(app_id, event, payload)
Handler->>DB: load App (vapid_key, vapid_email)
DB-->>Handler: VAPID credentials
Handler->>Handler: resolve subscriptions (dedupe)
loop per subscription
Handler->>PyWP: webpush(subscription_info, payload, vapid_claims)
alt success
PyWP-->>Handler: OK
else 404/410 (expired)
Handler->>DB: delete expired subscription
DB-->>Handler: deletion result
else error
PyWP-->>Handler: error
Handler->>Handler: log and continue
end
end
Handler-->>Client: summary results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| auth = self.field("auth", None) | ||
| unsubscribe = self.field("unsubscribe", False, cast=bool) | ||
| web_push = pushi.WebPush.new() | ||
| web_push = self.state.web_push_handler.subscribe( | ||
| web_push, auth=auth, unsubscribe=unsubscribe |
There was a problem hiding this comment.
The WebPush controller reads a single auth field (lines 78‑82) and passes it to web_push_handler.subscribe as the private-channel signature, but the WebPush model’s auth attribute (src/pushi/app/models/web_push.py:60‑65) is the Web Push encryption secret that must always be supplied. For private/presence/peer/personal channels this value is not a valid HMAC signature, so State.verify raises Invalid signature and the subscription cannot be created, with no way for clients to provide both the encryption secret and a channel auth token. This makes Web Push subscriptions to any private-style channel fail outright.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
AGENTS.md (1)
39-39: Minor inconsistency in docstring tag reference.The guideline mentions
:args:but the standard Sphinx tag is:param:. Looking at the new code inweb_push.py, it correctly uses:param:instead of:args:. Consider updating the guideline to match actual usage.-- Use Python docstrings with the `:type:`, `:args:`, `:rtype:`, `:return:`, etc. structure. +- Use Python docstrings with the `:type:`, `:param:`, `:rtype:`, `:return:`, etc. structure.src/pushi/base/web_push.py (1)
208-234: Consider usinglogger.exceptionfor full traceback capture.When catching exceptions, using
self.logger.exception()instead ofself.logger.error()will automatically include the full traceback, which aids debugging in production.except Exception as exception: # logs any other unexpected errors - self.logger.error( + self.logger.exception( "Unexpected error sending web push to '%s': %s" % (subscription_obj.endpoint, str(exception)) )Similarly for line 224-227 inside the delete exception handler.
src/pushi/app/models/app.py (1)
92-103: Consider tightening VAPID field metadata (optional)The
vapid_keyandvapid_emailfields are correctly introduced and match howWebPushHandlerconsumes them. As a small improvement, you might want to:
- Mark
vapid_emailwith an appropriatemeta(for example an email-specific meta if the admin UI supports it), and/or- Add minimal validation (non-empty, maybe format) once you see how these are managed operationally.
Not required for functionality, but would improve admin UX and configuration safety.
src/pushi/app/models/web_push.py (1)
74-85: Validation logic is solid; iterable unpacking is a minor optional cleanupThe
validate()method correctly enforces non‑null and non‑empty constraints on all required fields. If you want to silence the Ruff RUF005 warning and slightly modernize the code, you can switch to iterable unpacking:- def validate(cls): - return super(WebPush, cls).validate() + [ + def validate(cls): + return [ + *super(WebPush, cls).validate(), appier.not_null("endpoint"), appier.not_empty("endpoint"), appier.not_null("p256dh"), appier.not_empty("p256dh"), appier.not_null("auth"), appier.not_empty("auth"), appier.not_null("event"), appier.not_empty("event"), - ] + ]Purely cosmetic; current implementation is functionally fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
AGENTS.md(1 hunks)extra.txt(1 hunks)src/pushi/app/controllers/__init__.py(2 hunks)src/pushi/app/controllers/web_push.py(1 hunks)src/pushi/app/models/__init__.py(2 hunks)src/pushi/app/models/app.py(2 hunks)src/pushi/app/models/web_push.py(1 hunks)src/pushi/base/__init__.py(1 hunks)src/pushi/base/push_api.py(0 hunks)src/pushi/base/state.py(2 hunks)src/pushi/base/web_push.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/pushi/base/push_api.py
🧰 Additional context used
🧬 Code graph analysis (7)
src/pushi/base/__init__.py (1)
src/pushi/base/web_push.py (1)
WebPushHandler(43-425)
src/pushi/app/models/__init__.py (1)
src/pushi/app/models/web_push.py (1)
WebPush(36-110)
src/pushi/app/controllers/web_push.py (4)
src/pushi/app/models/base.py (1)
state(73-75)src/pushi/base/web_push.py (4)
subscriptions(295-314)unsubscribe(369-399)subscribe(316-367)unsubscribes(401-425)src/pushi/app/base.py (1)
auth(52-55)src/pushi/app/models/web_push.py (1)
WebPush(36-110)
src/pushi/app/models/web_push.py (2)
src/pushi/app/models/base.py (4)
PushiBase(35-88)get(39-44)state(73-75)app_id(78-82)src/pushi/base/web_push.py (2)
remove(276-293)add(255-274)
src/pushi/base/web_push.py (5)
src/pushi/base/handler.py (1)
Handler(32-50)src/pushi/app/models/base.py (6)
app_id(78-82)app_key(85-88)get(39-44)count(55-60)find(47-52)exists(63-65)src/pushi/base/state.py (5)
get_app(1143-1148)get_channels(1094-1096)load(96-163)subscribe(269-447)unsubscribe(449-591)src/pushi/app/models/web_push.py (1)
WebPush(36-110)src/pushi/app/controllers/web_push.py (1)
delete(103-121)
src/pushi/base/state.py (4)
src/pushi/base/web_push.py (2)
WebPushHandler(43-425)load(236-253)src/pushi/base/web.py (1)
load(138-144)src/pushi/base/handler.py (1)
load(49-50)src/pushi/base/apn.py (1)
load(200-206)
src/pushi/app/controllers/__init__.py (1)
src/pushi/app/controllers/web_push.py (1)
WebPushController(36-121)
🪛 Ruff (0.14.7)
src/pushi/app/controllers/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
src/pushi/app/models/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
76-85: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
src/pushi/base/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
60-60: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
130-130: Consider [event, *extra] instead of concatenation
Replace with [event, *extra]
(RUF005)
223-223: Do not catch blind exception: Exception
(BLE001)
224-227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
229-229: Do not catch blind exception: Exception
(BLE001)
231-234: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (18)
src/pushi/base/web_push.py (6)
37-58: LGTM!Good pattern for optional dependency handling with the try/except import block. The handler initialization is clean and follows the base Handler pattern.
119-159: LGTM!The message extraction logic with fallback chain and payload preparation is well implemented. The list concatenation on line 130 is appropriate for maintaining Python 2.7+ compatibility as per project guidelines.
236-274: LGTM!The
loadandaddmethods follow the same pattern as other handlers (APNHandler, WebHandler) for preloading subscriptions and managing the in-memory map.
276-293: LGTM!The remove method correctly removes the subscription from the in-memory map. The empty list/dict cleanup is a minor optimization that could be added later if memory becomes a concern.
316-367: LGTM!The subscribe method properly handles private channel authentication, prevents duplicate subscriptions, and reuses existing subscriptions when found. The logic mirrors the pattern used for WebSocket subscriptions in
state.py.
369-424: LGTM!The unsubscribe methods correctly handle both single and bulk deletion with optional event filtering. The
forceparameter appropriately controls error behavior when subscriptions are not found.src/pushi/base/state.py (2)
51-51: LGTM!Import follows the existing pattern for other handlers (apn, web).
165-176: LGTM!The WebPushHandler integration follows the established pattern for handlers: instantiate, load from database, then append to the handlers list. This ensures web push subscriptions are preloaded and the handler receives messages through the
send_channeldispatch loop.src/pushi/app/models/__init__.py (2)
35-35: LGTM!Module import follows the established pattern.
44-44: LGTM!WebPush export is consistent with other model exports in the package.
src/pushi/app/controllers/__init__.py (2)
35-35: LGTM!Module import follows the established pattern.
44-44: LGTM!WebPushController export is consistent with other controller exports in the package.
extra.txt (1)
2-2: LGTM!The pywebpush dependency is appropriately added to the optional extras file, consistent with the graceful degradation pattern in
WebPushHandlerthat handles the missing library via try/except import and validates availability before use. The library is actively maintained with the latest version at 2.1.2 (released November 2025).src/pushi/app/models/app.py (1)
40-69: Field metadata improvements for core App identifiers look goodThe expanded
observationsonname,ident,key, andsecretaccurately describe their purpose and generation, and align with the existingpre_createlogic. No functional issues here.src/pushi/base/__init__.py (1)
28-40: WebPushHandler export is consistent with existing handler wiringImporting
.web_pushand exposingWebPushHandlerat the package root mirrors the existing pattern forAPNHandler,WebHandler, etc. This keeps the public API coherent and makes the handler easily discoverable.src/pushi/app/controllers/web_push.py (2)
44-59: Listing subscriptions via state handler is straightforward and aligned
list()simply forwards optionalendpoint/eventfilters tostate.web_push_handler.subscriptions, returning the handler’s structured dict. This matches the handler contract and keeps the controller thin.
86-121: Delete endpoints correctly delegate to handler unsubscribe logicBoth
deletes()(all events for an endpoint) anddelete()(single endpoint/event pair) delegate toweb_push_handler.unsubscribes/unsubscribeand return mapped models or an empty dict. The use offorceindelete()gives callers control over not‑found behavior. No issues spotted here.src/pushi/app/models/web_push.py (1)
36-72: WebPush model schema aligns with Web Push subscription requirementsThe
endpoint,p256dh,auth, andeventfields, their metadata, and docstrings match the Web Push subscription payload and howWebPushHandlerbuildssubscription_info. Indexingendpointandeventalso fits the common query patterns.
| def send(self, app_id, event, json_d, invalid={}): | ||
| """ | ||
| Sends web push notifications to all subscribed endpoints for | ||
| the provided event/channel. | ||
| Uses the pywebpush library to send encrypted notifications | ||
| via the W3C Web Push protocol with VAPID authentication. | ||
| Automatically removes expired/invalid subscriptions. | ||
| :type app_id: String | ||
| :param app_id: The application identifier for which the | ||
| message is being sent. | ||
| :type event: String | ||
| :param event: The event/channel name to send the notification to. | ||
| :type json_d: Dictionary | ||
| :param json_d: The JSON data structure containing the notification | ||
| payload and metadata. | ||
| :type invalid: Dictionary | ||
| :param invalid: Map of already processed subscription IDs to avoid | ||
| duplicate sends (default: empty dict). | ||
| """ |
There was a problem hiding this comment.
Fix mutable default argument - shared state bug.
Using invalid={} as a default argument is a well-known Python pitfall. The same dictionary instance is reused across all calls where invalid is not explicitly provided, causing subscriptions marked as invalid in one notification to persist into subsequent unrelated notifications.
- def send(self, app_id, event, json_d, invalid={}):
+ def send(self, app_id, event, json_d, invalid=None):Then at the start of the method body (around line 81):
+ if invalid is None:
+ invalid = {}
+
# verifies if the pywebpush library is available, if notCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.7)
60-60: 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 around lines 60 to 80, the send method uses a
mutable default argument invalid={} which causes shared-state bugs across calls;
change the signature to use invalid=None and at the start of the method set
invalid = {} if invalid is None so each call gets a fresh dict; update any
internal references to invalid unchanged (no API behavior change) and run tests
to ensure no regressions.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive W3C Web Push API support to enable push notifications for web browsers, replacing the legacy Push API handler with a complete implementation using VAPID authentication and the pywebpush library.
- Introduces WebPush model, controller, and handler for managing web push subscriptions with encryption support
- Extends the App model with VAPID authentication fields (private key and email)
- Updates documentation to recommend Python docstring standards for future code consistency
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pushi/base/web_push.py | New handler implementing W3C Web Push protocol with subscription management, VAPID authentication, and automatic cleanup of expired subscriptions |
| src/pushi/base/state.py | Registers the new WebPushHandler and integrates it into application state lifecycle |
| src/pushi/base/push_api.py | Removed legacy Push API handler being replaced by new implementation |
| src/pushi/base/init.py | Updated imports to replace PushAPIHandler with WebPushHandler |
| src/pushi/app/models/web_push.py | New model for storing web push subscriptions with endpoint, encryption keys, and event information |
| src/pushi/app/models/app.py | Added VAPID key and email fields with comprehensive documentation for Web Push authentication |
| src/pushi/app/models/init.py | Registered new WebPush model for proper loading |
| src/pushi/app/controllers/web_push.py | New REST API controller providing endpoints for listing, creating, and deleting web push subscriptions |
| src/pushi/app/controllers/init.py | Registered new WebPushController for routing |
| extra.txt | Added pywebpush dependency for Web Push protocol support |
| AGENTS.md | Updated coding standards to recommend Python docstring format with type annotations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - auth: The authentication secret (required) | ||
| - event: The event/channel name (required) | ||
| - auth: Optional authentication token for private channels |
There was a problem hiding this comment.
Duplicate parameter name 'auth' in the docstring. The first 'auth' refers to the authentication token parameter for private channels, while the second appears to be duplicated. The parameter list should be corrected to remove the duplicate and ensure all parameters (endpoint, p256dh, auth, event, unsubscribe) are documented only once.
| - auth: The authentication secret (required) | |
| - event: The event/channel name (required) | |
| - auth: Optional authentication token for private channels | |
| - auth: The authentication secret (required); may also serve as an authentication token for private channels | |
| - event: The event/channel name (required) |
| :return: List of deleted WebPush model instances. | ||
| """ | ||
|
|
||
| kwargs = dict(endpoint=endpoint) |
There was a problem hiding this comment.
Inconsistency in the unsubscribes method: it uses dict(token=url) instead of dict(endpoint=endpoint) or dict(url=url). This appears to be a copy-paste error from another handler (likely the APN handler which uses tokens). The parameter should be endpoint=endpoint to match the endpoint parameter name and the database field.
|
|
||
| # adds the current subscription ID to the list of invalid items | ||
| # for the current message sending stream | ||
| invalid[subscription_id] = True |
There was a problem hiding this comment.
This expression mutates a default value.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/pushi/app/controllers/web_push.py (1)
68-75: Separate Web Push encryptionauthfrom channel auth token and fix param docsThe
createendpoint still overloads theauthfield for both the Web Push encryption secret and the private-channel auth token, and the docstring listsauthtwice. This will cause private/presence channel subscriptions to fail because the encryption secret is not a valid channel auth signature.Use distinct request fields: keep
authfor the Web Push encryption secret stored onWebPush, and introduce a separatetoken(or similar) for channel auth, passing that intosubscribe():@@ def create(self): - Form/JSON parameters: - - endpoint: The push service endpoint URL (required) - - p256dh: The P256DH encryption key (required) - - auth: The authentication secret (required) - - event: The event/channel name (required) - - auth: Optional authentication token for private channels - - unsubscribe: Whether to remove existing subscriptions (default: false) + Form/JSON parameters: + - endpoint: The push service endpoint URL (required) + - p256dh: The P256DH encryption key (required) + - auth: The authentication secret used for Web Push encryption (required) + - event: The event/channel name (required) + - token: Optional authentication token for private channels + - unsubscribe: Whether to remove existing subscriptions (default: false) @@ - auth = self.field("auth", None) + token = self.field("token", None) unsubscribe = self.field("unsubscribe", False, cast=bool) web_push = pushi.WebPush.new() web_push = self.state.web_push_handler.subscribe( - web_push, auth=auth, unsubscribe=unsubscribe + web_push, auth=token, unsubscribe=unsubscribe )This keeps the Web Push encryption
authclean on the model while allowing proper channel authentication.Also applies to: 80-85
🧹 Nitpick comments (3)
src/pushi/test/web_push.py (2)
346-349: Consider usingself.assertRaisesfor cleaner exception testing.While catching
Exceptionin tests is valid for verifying "no crash" behavior, it's overly broad. If specific behavior should not raise, consider:- try: - self.handler.send("app123", "notifications", json_d) - except Exception as e: - self.fail("Handler should not raise exception, but raised: %s" % str(e)) + # Should complete without raising any exception + self.handler.send("app123", "notifications", json_d)If
sendraises unexpectedly, the test will fail naturally with a clear traceback.
398-410: Remove or use the unusedresultvariable.The
resultvariable is assigned but never used.# Subscribe with auth token - result = self.handler.subscribe(mock_web_push, auth="test_auth_token") + self.handler.subscribe(mock_web_push, auth="test_auth_token")Alternatively, add an assertion to verify the result if that's the intent:
result = self.handler.subscribe(mock_web_push, auth="test_auth_token") self.assertEqual(result, mock_web_push)src/pushi/app/controllers/web_push.py (1)
1-2: Address Ruff EXE001: shebang on non-executable moduleRuff reports EXE001 because the file has a shebang but is not executable. For a library/controller module, the shebang is usually unnecessary.
If you want a clean Ruff run, consider removing the shebang:
-#!/usr/bin/python -# -*- coding: utf-8 -*- +# -*- coding: utf-8 -*-(or alternatively, make the file executable if you truly intend it to be run as a script).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
AGENTS.md(1 hunks)src/pushi/app/controllers/web_push.py(1 hunks)src/pushi/app/models/apn.py(1 hunks)src/pushi/app/models/subscription.py(1 hunks)src/pushi/app/models/web.py(1 hunks)src/pushi/app/models/web_push.py(1 hunks)src/pushi/test/web_push.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/pushi/test/web_push.py (4)
src/pushi/base/web_push.py (1)
WebPushHandler(43-425)src/pushi/base/state.py (3)
get_app(1143-1148)get_channels(1094-1096)verify(1201-1239)src/pushi/app/base.py (1)
auth(52-55)src/pushi/app/controllers/web_push.py (1)
delete(108-127)
src/pushi/app/models/apn.py (4)
src/pushi/app/models/base.py (1)
PushiBase(35-88)src/pushi/app/models/subscription.py (1)
post_create(61-65)src/pushi/app/models/web.py (1)
post_create(61-63)src/pushi/app/models/web_push.py (1)
post_create(98-100)
src/pushi/app/models/web.py (4)
src/pushi/app/models/base.py (1)
PushiBase(35-88)src/pushi/app/models/apn.py (1)
post_create(69-71)src/pushi/app/models/subscription.py (1)
post_create(61-65)src/pushi/app/models/web_push.py (1)
post_create(98-100)
src/pushi/app/models/web_push.py (4)
src/pushi/app/models/apn.py (5)
validate(42-48)pre_update(62-67)post_create(69-71)post_update(73-75)post_delete(77-81)src/pushi/app/models/subscription.py (5)
validate(42-48)pre_update(54-59)post_create(61-65)post_update(67-71)post_delete(73-77)src/pushi/app/models/web.py (5)
validate(42-48)pre_update(54-59)post_create(61-63)post_update(65-67)post_delete(69-71)src/pushi/base/web_push.py (2)
remove(276-293)add(255-274)
🪛 GitHub Actions: Main Workflow
src/pushi/test/web_push.py
[error] 32-32: ImportError: No module named mock while importing test module. Ensure the appropriate mock library is available (in Python 2.7 you may need 'mock'; in Python 3, use 'unittest.mock').
🪛 Ruff (0.14.7)
src/pushi/test/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
196-196: Unused method argument: mock_web_push_model
(ARG002)
212-212: Unused method argument: mock_web_push_model
(ARG002)
212-212: Unused method argument: mock_pywebpush
(ARG002)
324-324: Unused function argument: args
(ARG001)
348-348: Do not catch blind exception: Exception
(BLE001)
399-399: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
472-472: Unused method argument: mock_pywebpush
(ARG002)
507-507: Do not catch blind exception: Exception
(BLE001)
src/pushi/app/controllers/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
src/pushi/app/models/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
76-85: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
🔇 Additional comments (6)
AGENTS.md (1)
39-39: LGTM!The docstring style guide addition aligns with the project's documentation standards and will help maintain consistency across the codebase.
src/pushi/app/models/apn.py (1)
69-71: Good fix for the lifecycle hook.Correctly changed from
pre_createtopost_createto match the method name. This aligns with the pattern used in other models (Web,Subscription,WebPush) and ensures proper parent class lifecycle delegation.src/pushi/app/models/subscription.py (1)
61-65: Good fix for the lifecycle hook.Correctly delegates to
post_createinstead ofpre_create, consistent with the fix applied to other models in this PR.src/pushi/app/models/web.py (1)
61-63: Good fix for the lifecycle hook.Correctly delegates to
post_create, maintaining consistency across all model lifecycle hooks in this PR.src/pushi/app/models/web_push.py (1)
36-110: Well-structured model with correct lifecycle hooks.The
WebPushmodel follows the established patterns from other models (APN,Web,Subscription):
- Proper validation for all required fields
- Correct lifecycle hook delegation to parent class
- Proper synchronization with
web_push_handlerfor in-memory subscription trackingThe previous issue (calling
pre_createinpost_create) has been addressed.src/pushi/test/web_push.py (1)
45-510: Comprehensive test coverage for WebPushHandler.The test suite thoroughly covers:
- Initialization and attribute setup
- Subscription add/remove operations with deduplication
- Loading from database
- Filtering subscriptions
- Send flow with various scenarios (unavailable library, missing VAPID, success, exceptions)
- Subscribe/unsubscribe flows including private channel authentication
- Message extraction from different JSON formats
Good job on the test coverage for the new Web Push functionality.
…r both standard and PyPy builds
…n for regex patterns
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/pushi/app/controllers/web_push.py (1)
68-74: Duplicateauthparameter in docstring.The docstring lists
authtwice (lines 71 and 73) with different semantics: the Web Push encryption secret and a private channel authentication token. This confuses API consumers about what value to provide.Based on past review comments, clarify or consolidate the documentation:
Form/JSON parameters: - endpoint: The push service endpoint URL (required) - p256dh: The P256DH encryption key (required) - - auth: The authentication secret (required) + - auth: The authentication secret for Web Push encryption (required) - event: The event/channel name (required) - - auth: Optional authentication token for private channels + - token: Optional authentication token for private channels - unsubscribe: Whether to remove existing subscriptions (default: false)If a separate
tokenparameter is needed for private channel auth, ensure the implementation also reads it separately fromauth.
🧹 Nitpick comments (4)
.github/workflows/main.yml (1)
51-54: Dead code: Black check condition for PyPy.The
if: matrix.python-version == '3.12'condition will never evaluate to true since the PyPy matrix (line 42) only contains2.7. This step is effectively dead code.Consider removing the unreachable black check for PyPy:
- run: | pip install black black . --check - if: matrix.python-version == '3.12' + if: false # PyPy matrix doesn't include 3.12Or remove the step entirely since black formatting is already verified in the CPython matrix.
src/pushi/test/web_push.py (2)
310-387: Test isolation comment indicates uncertainty.The comment at lines 380-385 acknowledges uncertainty about test isolation, which is honest but suggests the test may not be fully verifying the expected behavior. The key assertion (handler doesn't crash) is valid, but consider adding explicit verification of the delete call when the exception path is triggered.
Consider restructuring to more explicitly verify the subscription deletion on 410 response:
# If webpush was called and raised 410, verify delete was called if mock_webpush_func.called: mock_subscription.delete.assert_called_once()
428-441: Unused variableresult.The
resultvariable is assigned but never used. Either use_to indicate intentionally unused, or assert on the result.# Subscribe with auth token - result = self.handler.subscribe(mock_web_push, auth="test_auth_token") + _ = self.handler.subscribe(mock_web_push, auth="test_auth_token")Or add an assertion:
result = self.handler.subscribe(mock_web_push, auth="test_auth_token") self.assertEqual(result, mock_web_push)src/pushi/base/web_push.py (1)
164-177: Database lookup inside loop may cause N+1 query problem.Each subscription triggers a database lookup via
pushi.WebPush.get(id=subscription_id). For high-volume channels, this could be inefficient.Consider batch-loading subscriptions before the loop if performance becomes a concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/main.yml(3 hunks)AGENTS.md(2 hunks)extra.txt(1 hunks)src/pushi/app/controllers/apn.py(1 hunks)src/pushi/app/controllers/subscription.py(1 hunks)src/pushi/app/controllers/web.py(1 hunks)src/pushi/app/controllers/web_push.py(1 hunks)src/pushi/app/models/web_push.py(1 hunks)src/pushi/base/web_push.py(1 hunks)src/pushi/test/web_push.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- extra.txt
🧰 Additional context used
🧬 Code graph analysis (1)
src/pushi/app/models/web_push.py (3)
src/pushi/app/models/base.py (4)
PushiBase(35-88)get(39-44)state(73-75)app_id(78-82)src/pushi/app/models/web.py (6)
validate(42-48)list_names(51-52)pre_update(54-59)post_create(61-63)post_update(65-67)post_delete(69-71)src/pushi/app/models/app.py (2)
validate(106-111)list_names(114-115)
🪛 GitHub Actions: Main Workflow
src/pushi/app/models/web_push.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted, 49 would be left unchanged. Run 'black --write' to fix code style issues.
🪛 Ruff (0.14.7)
src/pushi/app/models/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
78-87: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
src/pushi/app/controllers/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
src/pushi/base/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
60-60: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
130-130: Consider [event, *extra] instead of concatenation
Replace with [event, *extra]
(RUF005)
223-223: Do not catch blind exception: Exception
(BLE001)
224-227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
229-229: Do not catch blind exception: Exception
(BLE001)
231-234: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/pushi/test/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
199-199: Unused method argument: mock_web_push_model
(ARG002)
219-219: Unused method argument: mock_web_push_model
(ARG002)
353-353: Unused function argument: args
(ARG001)
377-377: Do not catch blind exception: Exception
(BLE001)
430-430: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
541-541: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (24)
src/pushi/app/controllers/web.py (1)
59-61: LGTM! Raw string for regex route pattern.Using a raw string (
r"...") ensures the backslash in[\.\w-]+is treated literally, preventing potential escape sequence issues. This is consistent with similar updates across other controllers.src/pushi/app/controllers/subscription.py (1)
63-65: LGTM! Consistent raw string update.Same raw string fix as in other controllers, ensuring the regex pattern is correctly interpreted.
.github/workflows/main.yml (1)
17-17: LGTM! Python 3.13 support added.Adding Python 3.13 to the test matrix aligns with modern Python support and the AGENTS.md compatibility requirement.
AGENTS.md (2)
22-22: LGTM! Simplified test command.The simplified
python setup.py testcommand is cleaner and consistent with the CI workflow.
38-38: LGTM! Docstring style guidance added.Clear documentation standards help maintain consistency across the codebase.
src/pushi/app/controllers/apn.py (1)
59-61: LGTM! Consistent raw string update.Same raw string fix as in other controllers, completing the consistent regex pattern handling across the codebase.
src/pushi/app/models/web_push.py (3)
47-74: LGTM! Well-structured field definitions.The fields properly capture Web Push subscription data with appropriate indexing on
endpointandeventfor efficient queries. The docstrings clearly explain each field's purpose.
76-87: LGTM! Comprehensive validation.All required fields are properly validated. The list concatenation pattern is consistent with other models in the codebase (e.g.,
web.py,app.py).
93-112: LGTM! Lifecycle hooks correctly implemented.The hooks properly maintain the in-memory subscription map, following the same pattern as
web.py. Thepre_updatecorrectly fetches the previous record to remove the old subscription before the new one is added inpost_update.src/pushi/test/web_push.py (5)
31-38: LGTM! Python 2.7/3.x compatible mock import.The try/except pattern correctly handles the mock import for both Python 2.7 (external
mockpackage) and Python 3.3+ (unittest.mock).
41-147: LGTM! Comprehensive subscription management tests.Good coverage of the handler's in-memory subscription management including edge cases like duplicates and removing non-existent subscriptions.
149-196: LGTM! Load and filter tests.Properly tests database loading and query filtering functionality.
443-500: LGTM! Unsubscribe tests.Good coverage of unsubscribe scenarios including not found and bulk operations.
502-546: LGTM! Message extraction robustness test.Valuable test that verifies the handler gracefully handles various JSON message formats without crashing.
src/pushi/app/controllers/web_push.py (3)
44-60: LGTM!The
listendpoint is well-structured with clear parameter handling and delegation to the handler.
88-101: LGTM!Clean implementation for bulk deletion by endpoint with proper response formatting.
103-124: LGTM!The
deletemethod properly handles theforceparameter and returns appropriate responses for both found and not-found cases.src/pushi/base/web_push.py (7)
56-58: LGTM!Clean initialization with proper parent call and in-memory subscription storage setup.
236-250: LGTM!Clean preload implementation that populates the in-memory subscription map from the database.
252-269: LGTM!Properly handles the nested dictionary structure with duplicate prevention.
271-286: LGTM!Clean removal logic from the in-memory subscription map.
288-307: LGTM!Clean filtering implementation with proper database query delegation.
345-354: Verify:unsubscribe=Trueremoves ALL subscriptions for the endpoint, not just duplicates.Line 347 calls
self.unsubscribe(web_push.endpoint, force=False)without passingevent, which removes all subscriptions for this endpoint across all events. If a client re-subscribes to one event, they lose subscriptions to other events.If the intent is to prevent duplicate endpoint+event pairs, pass the event:
# if unsubscribe is enabled, removes any existing subscriptions - # for the same endpoint (prevents duplicates) - unsubscribe and self.unsubscribe(web_push.endpoint, force=False) + # for the same endpoint+event (prevents duplicates) + unsubscribe and self.unsubscribe(web_push.endpoint, event=web_push.event, force=False)If removing all subscriptions for the endpoint is intentional (forcing single-event subscriptions), the docstring should clarify this behavior.
394-417: LGTM!Bulk deletion implementation correctly finds and removes all matching subscriptions.
| handler.Handler.__init__(self, owner, name="web_push") | ||
| self.subs = {} | ||
|
|
||
| def send(self, app_id, event, json_d, invalid={}): |
There was a problem hiding this comment.
Fix mutable default argument - shared state bug.
Using invalid={} as a default creates a single dictionary instance shared across all calls. When invalid[subscription_id] = True is set on line 206, it persists to subsequent calls, causing subscriptions to be incorrectly skipped.
This was flagged in a past review and remains unresolved.
- def send(self, app_id, event, json_d, invalid={}):
+ def send(self, app_id, event, json_d, invalid=None):Then at the start of the method body (after line 80):
if invalid is None:
invalid = {}🧰 Tools
🪛 Ruff (0.14.7)
60-60: 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 around line 60, the send method uses a mutable
default argument invalid={}, which creates shared state across calls; change the
signature to use invalid=None and at the start of the method (immediately after
the method docstring/around line 80) add a guard: if invalid is None: invalid =
{} so each call gets a fresh dict; ensure all call sites still work with None or
a dict and remove reliance on the previous mutable default.
| # checks if a subscription already exists for this endpoint and event | ||
| exists = pushi.WebPush.exists(endpoint=web_push.endpoint, event=web_push.event) | ||
| if exists: | ||
| web_push = exists | ||
| else: | ||
| web_push.save() | ||
|
|
||
| self.logger.debug( | ||
| "Subscribed '%s' for '%s'" % (web_push.endpoint, web_push.event) | ||
| ) | ||
|
|
||
| return web_push |
There was a problem hiding this comment.
In-memory subscription map not updated on subscribe.
When a new subscription is saved, self.add() is not called to update the in-memory self.subs map. The subscription won't receive notifications until load() is called again.
exists = pushi.WebPush.exists(endpoint=web_push.endpoint, event=web_push.event)
if exists:
web_push = exists
else:
web_push.save()
+ self.add(web_push.app_id, web_push.id, web_push.event)🤖 Prompt for AI Agents
In src/pushi/base/web_push.py around lines 349 to 360, the in-memory
subscription map (self.subs) is not updated when a new subscription is saved;
after saving a new web_push you must call self.add(web_push) so it becomes
available for notifications (optionally call self.add(exists) when an existing
subscription is loaded to ensure the map is always in sync). Update the
subscribe flow to invoke self.add(...) right after web_push.save() (and when
assigning from exists, ensure self.subs contains that entry).
| def unsubscribe(self, endpoint, event=None, force=True): | ||
| """ | ||
| Unsubscribes a web push endpoint from an event/channel. | ||
| :type endpoint: String | ||
| :param endpoint: The push endpoint URL to unsubscribe. | ||
| :type event: String | ||
| :param event: Optional event/channel name. If None, unsubscribes | ||
| from all events (default: None). | ||
| :type force: bool | ||
| :param force: Whether to raise an error if subscription not found | ||
| (default: True). | ||
| :rtype: WebPush | ||
| :return: The deleted WebPush model instance or None if not found. | ||
| """ | ||
|
|
||
| self.logger.debug("Unsubscribing '%s' from '%s'" % (endpoint, event or "*")) | ||
|
|
||
| kwargs = dict(endpoint=endpoint, raise_e=force) | ||
| if event: | ||
| kwargs["event"] = event | ||
|
|
||
| web_push = pushi.WebPush.get(**kwargs) | ||
| if not web_push: | ||
| return None | ||
|
|
||
| web_push.delete() | ||
|
|
||
| self.logger.debug("Unsubscribed '%s' for '%s'" % (endpoint, event or "*")) | ||
|
|
||
| return web_push |
There was a problem hiding this comment.
In-memory subscription map not updated on unsubscribe.
The unsubscribe() and unsubscribes() methods delete from the database but don't call self.remove() to update the in-memory self.subs map. This leaves stale subscription IDs that cause unnecessary database lookups and warnings in send().
web_push = pushi.WebPush.get(**kwargs)
if not web_push:
return None
+ self.remove(web_push.app_id, web_push.id, web_push.event)
web_push.delete()Similarly for unsubscribes():
web_pushes = pushi.WebPush.find(**kwargs)
for web_push in web_pushes:
+ self.remove(web_push.app_id, web_push.id, web_push.event)
web_push.delete()Also applies to: 394-417
🤖 Prompt for AI Agents
In src/pushi/base/web_push.py around lines 362-392 (and also apply same fix to
394-417), unsubscribe/unsubscribes remove DB records but never update the
in-memory self.subs map, leaving stale entries; after successfully deleting each
WebPush instance call self.remove(...) with the same identifier used to track
subscriptions (e.g. web_push.id or the endpoint/event key used by add/remove) to
remove it from self.subs; for batch unsubscribes iterate the deleted web_push
items and call self.remove for each so the in-memory map stays consistent with
the DB and avoids stale lookups/warnings.
Docstring inconsistent with implementation for event=None.
The docstring states "If None, unsubscribes from all events" but WebPush.get() returns only one record. This removes a single arbitrary subscription, not all subscriptions for the endpoint.
Either update the docstring to reflect reality, or use unsubscribes() logic:
:type event: String
- :param event: Optional event/channel name. If None, unsubscribes
- from all events (default: None).
+ :param event: Optional event/channel name. If None, removes the first
+ matching subscription for the endpoint (default: None).📝 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 unsubscribe(self, endpoint, event=None, force=True): | |
| """ | |
| Unsubscribes a web push endpoint from an event/channel. | |
| :type endpoint: String | |
| :param endpoint: The push endpoint URL to unsubscribe. | |
| :type event: String | |
| :param event: Optional event/channel name. If None, unsubscribes | |
| from all events (default: None). | |
| :type force: bool | |
| :param force: Whether to raise an error if subscription not found | |
| (default: True). | |
| :rtype: WebPush | |
| :return: The deleted WebPush model instance or None if not found. | |
| """ | |
| self.logger.debug("Unsubscribing '%s' from '%s'" % (endpoint, event or "*")) | |
| kwargs = dict(endpoint=endpoint, raise_e=force) | |
| if event: | |
| kwargs["event"] = event | |
| web_push = pushi.WebPush.get(**kwargs) | |
| if not web_push: | |
| return None | |
| web_push.delete() | |
| self.logger.debug("Unsubscribed '%s' for '%s'" % (endpoint, event or "*")) | |
| return web_push | |
| def unsubscribe(self, endpoint, event=None, force=True): | |
| """ | |
| Unsubscribes a web push endpoint from an event/channel. | |
| :type endpoint: String | |
| :param endpoint: The push endpoint URL to unsubscribe. | |
| :type event: String | |
| :param event: Optional event/channel name. If None, removes the first | |
| matching subscription for the endpoint (default: None). | |
| :type force: bool | |
| :param force: Whether to raise an error if subscription not found | |
| (default: True). | |
| :rtype: WebPush | |
| :return: The deleted WebPush model instance or None if not found. | |
| """ | |
| self.logger.debug("Unsubscribing '%s' from '%s'" % (endpoint, event or "*")) | |
| kwargs = dict(endpoint=endpoint, raise_e=force) | |
| if event: | |
| kwargs["event"] = event | |
| web_push = pushi.WebPush.get(**kwargs) | |
| if not web_push: | |
| return None | |
| web_push.delete() | |
| self.logger.debug("Unsubscribed '%s' for '%s'" % (endpoint, event or "*")) | |
| return web_push |
🤖 Prompt for AI Agents
In src/pushi/base/web_push.py around lines 362 to 392, the docstring claims "If
None, unsubscribes from all events" but the implementation calls
WebPush.get(...) which only returns a single record; update either the docstring
to state it only removes a single subscription for the endpoint (and keep
current behavior), or change the implementation to delete all matching
subscriptions: query all WebPush records matching endpoint (and event if
provided), delete each record, and return a list of deleted records (or None if
none found); also update the return type in the docstring accordingly and
preserve logging behavior for each deleted record.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pushi/app/models/web_push.py (1)
76-87: Consider iterable unpacking for validator concatenation.The current list concatenation works correctly. However, you can use iterable unpacking for a more modern Python style:
Apply this diff:
@classmethod def validate(cls): - return super(WebPush, cls).validate() + [ + return [ + *super(WebPush, cls).validate(), appier.not_null("endpoint"), appier.not_empty("endpoint"), appier.not_null("p256dh"),Based on static analysis hint (Ruff RUF005).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pushi/app/models/web_push.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pushi/app/models/web_push.py (2)
src/pushi/app/models/base.py (4)
PushiBase(35-88)get(39-44)state(73-75)app_id(78-82)src/pushi/base/web_push.py (2)
remove(271-286)add(252-269)
🪛 Ruff (0.14.7)
src/pushi/app/models/web_push.py
1-1: Shebang is present but file is not executable
(EXE001)
78-87: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
🔇 Additional comments (3)
src/pushi/app/models/web_push.py (3)
47-74: LGTM! Well-documented field definitions.The field definitions are appropriate for Web Push subscriptions:
- Proper indexing on
endpointandeventfor efficient lookups- Correct
longtextmeta for base64url-encoded encryption keys- Clear observations explaining each field's purpose
89-91: LGTM! Appropriate list display fields.Returning
endpointandeventfor list views is the right choice—these are the identifying fields, whilep256dhandauthare encryption secrets that don't need to be displayed.
93-112: LGTM! Lifecycle hooks correctly maintain subscription state.The lifecycle hooks properly synchronize the database with the in-memory subscription map:
pre_updateremoves the old subscription before changespost_create,post_update, andpost_deleteupdate the handler after DB operations- Conditional execution (
self.state and ...) safely handles cases where state might be None- The previous issue (line 101 calling wrong base method) has been correctly fixed
| class WebPush(base.PushiBase): | ||
| """ | ||
| Database model for W3C Web Push API subscriptions. | ||
| Stores the subscription information required to send | ||
| push notifications to web browsers, including the | ||
| push service endpoint and encryption keys. | ||
| :see: https://w3c.github.io/push-api | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
black --check src/pushi/app/models/web_push.pyRepository: hivesolutions/pushi
Length of output: 108
🏁 Script executed:
cat -A src/pushi/app/models/web_push.py | sed -n '36,45p'Repository: hivesolutions/pushi
Length of output: 395
🏁 Script executed:
# Check if there are actual trailing spaces before the line endings
sed -n '36,45p' src/pushi/app/models/web_push.py | od -c | head -50Repository: hivesolutions/pushi
Length of output: 103
Fix line endings: convert from CRLF to LF.
This file has Windows-style CRLF line endings (\r\n) instead of Unix-style LF endings (\n). Black enforces consistent line endings and will fail the formatting check. Run git config core.autocrlf to verify your Git configuration, then convert the file using your editor or a tool like dos2unix, and recommit before merging.
🤖 Prompt for AI Agents
In src/pushi/app/models/web_push.py around lines 36 to 45 the file uses Windows
CRLF line endings which will fail Black's formatting check; convert the file to
Unix LF line endings (e.g., run dos2unix src/pushi/app/models/web_push.py or
change your editor's EOL setting to LF), verify your Git config with git config
core.autocrlf and set it appropriately (false or input) to prevent
reintroduction, stage and recommit the file, and push the commit so the CI/Black
check will pass.
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)
.github/workflows/deploy.yml (1)
11-11: Unreachable condition: Black linting step will never execute in this workflow.The matrix at line 11 specifies only
python-version: [2.7], but the condition at line 24 requiresmatrix.python-version == '3.13'. These constraints are mutually exclusive—no job will ever satisfy both conditions simultaneously, so the Black linting step is dead code.To fix, choose one of:
- Add Python 3.13 to the matrix if linting should run on that version
- Remove the condition if linting should run on Python 2.7
- Update the condition to match an available matrix version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/deploy.yml(1 hunks).github/workflows/main.yml(3 hunks)
🔇 Additional comments (2)
.github/workflows/main.yml (2)
17-17: LGTM on Python 3.13 addition and Black check targeting.Adding Python 3.13 to the test matrix and updating Black to run on 3.13 aligns with the PR's goal to support newer Python versions. Running code formatting checks on a single representative version is a reasonable CI optimization.
Also applies to: 32-32
37-37: Verify test coverage for older Python versions.The changes exclude Python 3.9+ and 3.13 from
setup.py test(lines 37, 59), meaning only 2.7, 3.6, 3.7, and 3.8 run the older test harness. With PyPy reducing to[2.7]only (line 42), ensure that this reduced test matrix still covers Web Push functionality adequately.Does the PyPy 2.7 build need to validate Web Push features, or is it acceptable to skip full test coverage for end-of-life Python versions?
Also applies to: 42-42, 59-59
| pip install black | ||
| black . --check | ||
| if: matrix.python-version == '3.12' | ||
| if: matrix.python-version == '3.13' |
There was a problem hiding this comment.
Fix Black check condition in PyPy job—currently dead code.
The PyPy job's matrix contains only [2.7] (line 42), but the Black check condition at line 54 requires matrix.python-version == '3.13'. This condition will never be true in the PyPy job, resulting in Black never running for PyPy builds.
This appears to be a copy-paste oversight from the main build job. Black checks are typically not runtime-specific and should either be removed from the PyPy job or the condition updated.
Recommended fix: Remove the Black check from the PyPy job, since code formatting is validated in the main build job and is not PyPy-specific.
- run: |
pip install black
black . --check
- if: matrix.python-version == '3.13'
- run: |
pip install pytest
pytestAlternatively, if the Black check should remain, remove the if condition entirely (though this is redundant with the main build job).
📝 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.
| if: matrix.python-version == '3.13' | |
| - run: | | |
| pip install black | |
| black . --check | |
| - run: | | |
| pip install pytest | |
| pytest |
🤖 Prompt for AI Agents
.github/workflows/main.yml around line 54: the PyPy job's matrix only contains
Python 2.7 so the step guarded by "if: matrix.python-version == '3.13'" is dead
code and Black will never run for PyPy; remove the Black check step from the
PyPy job entirely (preferred) by deleting that step block, or if you need to
keep the step for some reason, remove the conditional so it always runs in the
PyPy job.
This pull request adds support for the W3C Web Push API to the project, enabling push notifications for web browsers. The main changes include introducing a new
WebPushmodel, controller, and handler, extending theAppmodel for VAPID authentication, and updating the codebase to register and manage web push subscriptions. Additionally, documentation and dependency updates were made to support this new feature.Web Push API Support
WebPushmodel (src/pushi/app/models/web_push.py) to store web push subscriptions, including endpoint, encryption keys, and event/channel information.WebPushController(src/pushi/app/controllers/web_push.py) to provide REST API endpoints for listing, creating, and deleting web push subscriptions.WebPushHandlerand integrated it into the application state for managing subscriptions (src/pushi/base/web_push.py, updates insrc/pushi/base/state.py, and registration insrc/pushi/base/__init__.py). [1] [2] [3]App Model Enhancements
Appmodel to includevapid_keyandvapid_emailfields for VAPID authentication required by the Web Push protocol.Dependency and Documentation Updates
pywebpushlibrary to dependencies for handling Web Push encryption and protocol.AGENTS.md) to recommend Python docstring standards for future code consistency.Refactoring and Cleanup
src/pushi/base/push_api.py) and its registration, as it is superseded by the new Web Push implementation. [1] [2]Codebase Integration
__init__.pyfiles to ensure proper loading and routing. [1] [2] [3] [4]Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.