Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConverted many ad-hoc JSON payload constructions to typed Request models (.to_dict()), extended numerous public method signatures with new optional parameters and payload types, added video recording and participant-session endpoints, introduced webhook parsing utilities and tests, and propagated notification/notification-control flags across Feeds and Chat APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…Start/StopRecording previously)
…nd use correct Start/StopRecording endpoint
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@getstream/video/rtc/coordinator/ws.py`:
- Line 154: Replace the runtime-only assertion with an explicit guard that
survives Python -O: check that self._websocket (typed
Optional[ClientConnection]) is not None and if it is None raise an explicit
exception (e.g., RuntimeError or ValueError) with a clear message; update the
code path around the assert self._websocket is not None to perform this check
and then proceed using self._websocket as a non-None ClientConnection.
| self._build_auth_payload(), | ||
| ) | ||
| self._logger.debug("WebSocket connection established") | ||
| assert self._websocket is not None |
There was a problem hiding this comment.
Replace assert with an explicit guard — assert is stripped under python -O.
Since self._websocket is typed Optional[ClientConnection], the assertion is here purely for type narrowing. However, assert statements are silently removed when Python runs with the -O (optimize) flag, which can affect production deployments. Prefer an explicit guard that survives optimization:
🛡️ Proposed fix
- assert self._websocket is not None
+ if self._websocket is None:
+ raise StreamWSConnectionError("WebSocket connection was not established after connect()")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@getstream/video/rtc/coordinator/ws.py` at line 154, Replace the runtime-only
assertion with an explicit guard that survives Python -O: check that
self._websocket (typed Optional[ClientConnection]) is not None and if it is None
raise an explicit exception (e.g., RuntimeError or ValueError) with a clear
message; update the code path around the assert self._websocket is not None to
perform this check and then proceed using self._websocket as a non-None
ClientConnection.
…nd use correct Start/StopRecording endpoint
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@getstream/tests/test_webhook.py`:
- Around line 17-56: Replace the class-level setup_method in
TestVerifyWebhookSignature with pytest fixtures: create fixtures named secret,
body, and valid_signature that compute the same values (secret string, body
bytes, and HMAC using hashlib.sha256) and have tests accept those fixtures as
parameters; update each test method to remove self and use the fixture names
(e.g., def test_valid_signature(body, valid_signature, secret): assert
verify_webhook_signature(body, valid_signature, secret) is True) so
verify_webhook_signature is exercised via injected fixtures instead of instance
state.
In `@getstream/video/rest_client.py`:
- Around line 625-636: The get_call_participant_session_metrics function passes
datetime objects (since/until) directly into build_query_param which falls back
to json.dumps and will raise for datetimes; convert/serialize since and until to
an ISO-8601 string (or desired timestamp format) before calling
build_query_param (e.g., transform the values if not None) so build_query_param
receives strings; update the query_params creation in
get_call_participant_session_metrics to use the serialized values when calling
build_query_param.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
getstream/chat/async_rest_client.pygetstream/chat/rest_client.pygetstream/common/async_rest_client.pygetstream/common/rest_client.pygetstream/feeds/feeds.pygetstream/feeds/rest_client.pygetstream/models/__init__.pygetstream/moderation/async_rest_client.pygetstream/moderation/rest_client.pygetstream/tests/test_webhook.pygetstream/video/async_rest_client.pygetstream/video/rest_client.pygetstream/webhook.pytests/test_video_examples.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_video_examples.py
Comment on lines
+17
to
+56
| class TestVerifyWebhookSignature: | ||
| def setup_method(self): | ||
| self.secret = "test-webhook-secret" | ||
| self.body = b'{"type":"test.event"}' | ||
| self.valid_signature = hmac.new( | ||
| self.secret.encode("utf-8"), self.body, hashlib.sha256 | ||
| ).hexdigest() | ||
|
|
||
| def test_valid_signature(self): | ||
| assert ( | ||
| verify_webhook_signature(self.body, self.valid_signature, self.secret) | ||
| is True | ||
| ) | ||
|
|
||
| def test_wrong_signature(self): | ||
| assert ( | ||
| verify_webhook_signature(self.body, "invalidsignature", self.secret) | ||
| is False | ||
| ) | ||
|
|
||
| def test_tampered_body(self): | ||
| assert ( | ||
| verify_webhook_signature( | ||
| b'{"type":"tampered"}', self.valid_signature, self.secret | ||
| ) | ||
| is False | ||
| ) | ||
|
|
||
| def test_wrong_secret(self): | ||
| assert ( | ||
| verify_webhook_signature(self.body, self.valid_signature, "wrong-secret") | ||
| is False | ||
| ) | ||
|
|
||
| def test_string_body(self): | ||
| body_str = '{"type":"test.event"}' | ||
| sig = hmac.new( | ||
| self.secret.encode("utf-8"), body_str.encode("utf-8"), hashlib.sha256 | ||
| ).hexdigest() | ||
| assert verify_webhook_signature(body_str, sig, self.secret) is True |
There was a problem hiding this comment.
Replace setup_method with pytest fixtures for shared webhook inputs.
This file uses instance state instead of fixtures for shared data; please switch to fixtures for the secret/body/signature setup.
♻️ Suggested fixture-based setup
-class TestVerifyWebhookSignature:
- def setup_method(self):
- self.secret = "test-webhook-secret"
- self.body = b'{"type":"test.event"}'
- self.valid_signature = hmac.new(
- self.secret.encode("utf-8"), self.body, hashlib.sha256
- ).hexdigest()
+@pytest.fixture
+def webhook_secret():
+ return "test-webhook-secret"
+
+@pytest.fixture
+def webhook_body():
+ return b'{"type":"test.event"}'
+
+@pytest.fixture
+def webhook_signature(webhook_secret, webhook_body):
+ return hmac.new(
+ webhook_secret.encode("utf-8"), webhook_body, hashlib.sha256
+ ).hexdigest()
+
+class TestVerifyWebhookSignature:
+ def test_valid_signature(self, webhook_body, webhook_signature, webhook_secret):
+ assert (
+ verify_webhook_signature(webhook_body, webhook_signature, webhook_secret)
+ is True
+ )As per coding guidelines, “Use fixtures to inject objects in tests; test client fixtures can use the Stream API client”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@getstream/tests/test_webhook.py` around lines 17 - 56, Replace the
class-level setup_method in TestVerifyWebhookSignature with pytest fixtures:
create fixtures named secret, body, and valid_signature that compute the same
values (secret string, body bytes, and HMAC using hashlib.sha256) and have tests
accept those fixtures as parameters; update each test method to remove self and
use the fixture names (e.g., def test_valid_signature(body, valid_signature,
secret): assert verify_webhook_signature(body, valid_signature, secret) is True)
so verify_webhook_signature is exercised via injected fixtures instead of
instance state.
Comment on lines
+625
to
+636
| def get_call_participant_session_metrics( | ||
| self, | ||
| type: str, | ||
| id: str, | ||
| session: str, | ||
| user: str, | ||
| user_session: str, | ||
| since: Optional[datetime] = None, | ||
| until: Optional[datetime] = None, | ||
| ) -> StreamResponse[GetCallParticipantSessionMetricsResponse]: | ||
| query_params = build_query_param(**{"since": since, "until": until}) | ||
| path_params = { |
There was a problem hiding this comment.
Serialize since/until before building query params.
build_query_param falls back to json.dumps for unknown types, and datetime isn’t JSON‑serializable, so this can raise at runtime when since/until are provided.
🛠️ Proposed fix
- query_params = build_query_param(**{"since": since, "until": until})
+ query_params = build_query_param(
+ **{
+ "since": since.isoformat() if since else None,
+ "until": until.isoformat() if until else None,
+ }
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@getstream/video/rest_client.py` around lines 625 - 636, The
get_call_participant_session_metrics function passes datetime objects
(since/until) directly into build_query_param which falls back to json.dumps and
will raise for datetimes; convert/serialize since and until to an ISO-8601
string (or desired timestamp format) before calling build_query_param (e.g.,
transform the values if not None) so build_query_param receives strings; update
the query_params creation in get_call_participant_session_metrics to use the
serialized values when calling build_query_param.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Enhancements
Tests