Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
+ Coverage 79.93% 80.14% +0.21%
==========================================
Files 104 106 +2
Lines 8576 8678 +102
Branches 628 633 +5
==========================================
+ Hits 6855 6955 +100
Misses 1588 1588
- Partials 133 135 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds an optional connection tracking hook API to PyView’s WebSocket lifecycle so integrators can observe connects/disconnects and event handling timing.
Changes:
- Introduces a
ConnectionTrackerruntime-checkableProtocoland re-exports it frompyview. - Threads an optional
connection_trackerthroughPyView→LiveSocketHandlerand emits connect/event/disconnect callbacks. - Adds a
PyView.registered_routesconvenience property plus initial unit tests for plumbing/protocol conformance.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pyview/connection_tracker.py |
Defines the ConnectionTracker protocol for lifecycle callbacks. |
pyview/ws_handler.py |
Wires tracker callbacks into WebSocket connect/event/disconnect flow. |
pyview/pyview.py |
Accepts connection_tracker, passes it to handler, adds registered_routes. |
pyview/__init__.py |
Exports ConnectionTracker in the public package API. |
tests/test_connection_tracker.py |
Adds tests for protocol runtime-checkability and plumbing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.connection_tracker: | ||
| with suppress(Exception): | ||
| self.connection_tracker.on_connect( | ||
| topic, socket, lv_class, url.path, session | ||
| ) | ||
|
|
There was a problem hiding this comment.
on_connect is only invoked for the initial phx_join in handle(). When the client navigates within the same WebSocket (the phx_join handled inside _handle_connected_loop), a new LiveView is mounted and first-rendered but the tracker never receives on_connect, so connection accounting will be incomplete. Consider invoking connection_tracker.on_connect(...) for navigation joins as well (and likewise for any mount path that results in a new ConnectedLiveViewSocket).
|
|
||
| await self.manager.send_personal_message(json.dumps(resp), websocket) | ||
|
|
||
| if self.connection_tracker: |
There was a problem hiding this comment.
Use if self.connection_tracker is not None: rather than a truthiness check. As written, a tracker instance that defines __bool__/__len__ and evaluates to False would silently disable tracking.
| if self.connection_tracker: | |
| if self.connection_tracker is not None: |
| if self.connection_tracker and topic: | ||
| with suppress(Exception): | ||
| self.connection_tracker.on_disconnect(topic) |
There was a problem hiding this comment.
on_disconnect(topic) is called here on phx_leave, but handle() also unconditionally calls on_disconnect(topic) in its finally block when the WebSocket closes. If the client sends phx_leave and then disconnects without re-joining a new topic, the tracker will receive a duplicate disconnect for the same topic. Consider clearing/resetting the tracked topic (or tracking an "already disconnected" flag) after phx_leave so the finally block doesn’t emit a second disconnect for the same view.
| if self.connection_tracker and topic: | |
| with suppress(Exception): | |
| self.connection_tracker.on_disconnect(topic) |
| def test_pyview_passes_tracker_to_handler(): | ||
| """PyView should pass connection_tracker through to LiveSocketHandler.""" | ||
| tracker = FakeTracker() | ||
| app = PyView(connection_tracker=tracker) | ||
| assert app.live_handler.connection_tracker is tracker | ||
|
|
There was a problem hiding this comment.
These tests validate protocol conformance and that the tracker is plumbed through, but they don’t assert the new runtime behavior (that on_connect, on_event, and on_disconnect are actually invoked by LiveSocketHandler, and that exceptions from the tracker are suppressed). Adding an async WebSocket handler test (similar to existing test_ws_handler_cleanup.py) that drives a phx_join + event + disconnect (and a phx_leave path) would prevent regressions in the hook semantics.
ogrodnek
force-pushed
the
add_connection_hooks
branch
from
7cd7075 to
0ae5b80
Compare
![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.
Reviewed commit: f398c4123d
ℹ️ 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".
| if self.connection_tracker and topic: | ||
| with suppress(Exception): | ||
| self.connection_tracker.on_disconnect(topic) |
There was a problem hiding this comment.
The finally block always emits on_disconnect(topic) using the topic captured from the initial phx_join, but _handle_connected_loop can switch LiveViews via later phx_join messages. In a navigation flow (phx_leave old topic -> phx_join new topic -> socket closes), this reports the old topic again and never reports disconnect for the active topic, which can leave trackers with incorrect connection state/counts.
Useful? React with 👍 / 👎.
No description provided.