Use forked twirp codegen lib #187
Conversation
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
changed the title
WalkthroughReplaced the embedded async Twirp client with a new AsyncTwirpClient inside the generated signal_twirp module and converted signal RPC client methods to async; removed the embedded client file and its tests; regenerated protobuf artifacts (version header and descriptor offsets) with added enums/messages/fields; updated build scripts, workflow, and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant SignalClient as AsyncSignalServerClient
participant TwirpClient as AsyncTwirpClient
participant HTTP as aiohttp.Session
Client->>SignalClient: await Method(ctx, request)
SignalClient->>TwirpClient: await _make_request(url, ctx, request, response_obj, session)
TwirpClient->>HTTP: POST protobuf bytes (Content-Type: application/protobuf)
activate HTTP
alt 200 OK
HTTP-->>TwirpClient: protobuf bytes
TwirpClient-->>SignalClient: parsed response object
SignalClient-->>Client: return response
else Error / timeout
HTTP-->>TwirpClient: error body / timeout
TwirpClient-->>SignalClient: raise Twirp exception (mapped code/metadata)
SignalClient-->>Client: exception propagated
end
deactivate HTTP
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
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 |
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
changed the title
…-py into unfudge-python-dep-hell
tbarbugli
had a problem deploying
to
feeds-v3-ci
GitHub Actions
Failure
tbarbugli
had a problem deploying
to
feeds-v3-ci
GitHub Actions
Failure
tbarbugli
had a problem deploying
to
feeds-v3-ci
GitHub Actions
Failure
tbarbugli
had a problem deploying
to
feeds-v3-ci
GitHub Actions
Failure
tbarbugli
changed the title
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
generate_webrtc.sh(1 hunks)getstream/video/rtc/pb/stream/video/sfu/event/events_pb2.py(4 hunks)getstream/video/rtc/pb/stream/video/sfu/event/events_pb2.pyi(5 hunks)getstream/video/rtc/pb/stream/video/sfu/models/models_pb2.py(4 hunks)getstream/video/rtc/pb/stream/video/sfu/models/models_pb2.pyi(10 hunks)getstream/video/rtc/pb/stream/video/sfu/signal_rpc/signal_pb2.py(2 hunks)getstream/video/rtc/pb/stream/video/sfu/signal_rpc/signal_twirp.py(3 hunks)getstream/video/rtc/twirp_async_client_embed.py(0 hunks)getstream/video/rtc/twirp_client_wrapper.py(0 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (2)
- getstream/video/rtc/twirp_async_client_embed.py
- getstream/video/rtc/twirp_client_wrapper.py
✅ Files skipped from review due to trivial changes (1)
- getstream/video/rtc/pb/stream/video/sfu/signal_rpc/signal_pb2.py
🧰 Additional context used
🧬 Code graph analysis (2)
getstream/video/rtc/pb/stream/video/sfu/event/events_pb2.pyi (1)
getstream/video/rtc/pb/stream/video/sfu/models/models_pb2.pyi (1)
ParticipantSource(136-139)
getstream/video/rtc/pb/stream/video/sfu/models/models_pb2.pyi (1)
getstream/video/rtc/pb/stream/video/sfu/event/events_pb2.pyi (16)
ClearField(239-291)ClearField(347-352)ClearField(395-397)ClearField(420-425)ClearField(443-448)ClearField(463-465)ClearField(504-516)ClearField(543-548)ClearField(580-585)ClearField(619-631)ClearField(668-682)ClearField(787-817)ClearField(868-886)ClearField(923-933)ClearField(969-981)ClearField(1005-1010)
⏰ 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). (4)
- GitHub Check: Tests (3.12)
- GitHub Check: Tests (3.11)
- GitHub Check: Tests (3.10)
- GitHub Check: Tests (3.13)
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
tbarbugli
temporarily deployed
to
feeds-v3-ci
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
getstream/video/rtc/pb/stream/video/sfu/signal_rpc/signal_twirp.py (1)
253-255: Fix aiohttp context manager usage (already flagged).This issue has already been identified in a previous review:
session.post()returns an async context manager, not a coroutine. Usingasync with await session.post(...)will cause a runtime error becauseClientResponsedoesn't implement__aenter__.Apply this diff to fix:
- async with await session.post( - url=full_url, data=request.SerializeToString(), **kwargs - ) as resp: + async with session.post( + url=full_url, data=request.SerializeToString(), **kwargs + ) as resp:
🧹 Nitpick comments (1)
generate_webrtc.sh (1)
58-59: Clarify intent of unconditionalgo installon every run.The script now installs
protoc-gen-twirpyunconditionally every time it runs. For local development, this could be slow; for CI, this is acceptable but worth documenting.Consider adding a comment explaining this is intentional to ensure the pinned commit is always used, or add an optional
--skip-installflag if regeneration is frequent during development.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
generate_webrtc.sh(1 hunks)getstream/video/rtc/pb/stream/video/sfu/signal_rpc/signal_twirp.py(3 hunks)
🔇 Additional comments (5)
getstream/video/rtc/pb/stream/video/sfu/signal_rpc/signal_twirp.py (4)
5-19: LGTM! Imports are appropriate for async Twirp client.The new imports (asyncio, aiohttp, json, Optional, twirp exceptions/errors, urljoin) are all necessary for the AsyncTwirpClient implementation and properly support async HTTP operations, error handling, and URL construction.
221-228: LGTM! Initialization handles URL normalization correctly.The constructor properly normalizes the address by ensuring it ends with a slash, which is necessary for
urljointo work correctly when combining with relative paths.
230-249: LGTM! Request setup and URL construction are well-implemented.The method correctly:
- Merges headers from context and kwargs
- Sets the appropriate protobuf Content-Type
- Handles optional session parameter with proper tracking
- Constructs full URLs using
urljoinwith correct slash normalization
285-408: LGTM! AsyncSignalServerClient methods are correctly implemented.All RPC methods are properly converted to async and consistently delegate to
await self._make_request()with correct parameter forwarding. The implementation follows a uniform pattern across all nine methods (SetPublisher, SendAnswer, IceTrickle, UpdateSubscriptions, UpdateMuteStates, IceRestart, SendStats, StartNoiseCancellation, StopNoiseCancellation).generate_webrtc.sh (1)
52-56: Original PATH check at lines 52-56 is correct and necessary; review comment is incorrect.The verification shows:
PATH check is necessary: You must have GOPATH/bin (or GOBIN) in PATH to run installed commands. The pre-validation approach prevents wasted work.
No fragility in grep logic: Testing PATH matching edge cases (trailing slashes, substring positions) confirms the grep logic works correctly without false positives or false negatives.
Original approach is superior: Validating the environment BEFORE
go install(line 59) is better than checking tool availability after installation. If the PATH check fails, the script exits early; the suggested reactive check would only catch failures aftergo installhas already run.The script correctly validates that
$(go env GOPATH)/binis in PATH before proceeding withgo install, aligning with Go best practices.Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Chores
Breaking/Compatibility