-
Notifications
You must be signed in to change notification settings - Fork 137
Organize & document local participant #846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe pull request substantially expands the Changes
Sequence Diagram(s)sequenceDiagram
participant LocalParticipant
participant RpcRegistry as RPC Registry
participant RpcHandler as Remote Handler
participant NetworkLayer as Network/Signaling
LocalParticipant->>LocalParticipant: perform_rpc(data)
LocalParticipant->>LocalParticipant: publish_rpc_request()
LocalParticipant->>NetworkLayer: Send RPC Request
NetworkLayer->>RpcHandler: Route to handler
RpcHandler->>RpcRegistry: resolve_rpc_method()
RpcRegistry->>RpcHandler: Execute handler callback
RpcHandler->>NetworkLayer: Send RPC Response
NetworkLayer->>LocalParticipant: Deliver response payload
LocalParticipant->>LocalParticipant: handle_incoming_rpc_response()
LocalParticipant->>NetworkLayer: Send RPC Ack
NetworkLayer->>RpcHandler: Ack received
sequenceDiagram
participant App as Application
participant LocalParticipant
participant StreamWriter as Stream Writer
participant DataPublisher as Data Publisher
participant RemoteParticipant
App->>LocalParticipant: send_text(text, options)
LocalParticipant->>StreamWriter: Create TextStreamWriter
StreamWriter->>DataPublisher: publish_data(packet)
DataPublisher->>RemoteParticipant: Send via data channel
RemoteParticipant-->>App: Text received
App->>LocalParticipant: stream_bytes(options)
LocalParticipant->>StreamWriter: Create ByteStreamWriter
StreamWriter->>DataPublisher: publish_raw_data(packet)
DataPublisher->>RemoteParticipant: Send via data channel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (14)
🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
livekit/src/room/participant/local_participant.rs (3)
239-301: Preserve existing attributes when updating name/metadata.
Bothset_nameandset_metadatasendattributes: Default::default(), which likely clears existing attributes on the server. That’s a user-visible regression.✅ Suggested fix
- attributes: Default::default(), + attributes: self.attributes(),- attributes: Default::default(), + attributes: self.attributes(),
932-1004: Avoid panics when no RoomSession is available.
These public methods callself.session().unwrap(). If a caller uses the local participant before session setup or after disconnect, this will panic. Consider returning aStreamResulterror instead of unwrapping.
645-654: Consider graceful error handling for server version parsing.
While LiveKit's server guaranteesserver_info.versionfollows Semantic Versioning format (including snapshot builds like "1.9.2-SNAPSHOT"), usingunwrap()is unnecessarily fragile. Handle parse errors defensively to improve robustness against unexpected server behavior or edge cases.✅ Suggested fix
- let server_version = Version::parse(&server_info.version).unwrap(); - let min_required_version = Version::parse("1.8.0").unwrap(); - if server_version < min_required_version { - return Err(RpcError::built_in(RpcErrorCode::UnsupportedServer, None)); - } + let min_required_version = Version::parse("1.8.0").unwrap(); + match Version::parse(&server_info.version) { + Ok(server_version) if server_version < min_required_version => { + return Err(RpcError::built_in(RpcErrorCode::UnsupportedServer, None)); + } + Ok(_) => {} + Err(err) => { + log::warn!( + "Unparseable LiveKit server version '{}': {}", + server_info.version, + err + ); + } + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
livekit/src/room/participant/local_participant.rs
⏰ 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). (14)
- GitHub Check: Build (x86_64-linux-android)
- GitHub Check: Build (x86_64-unknown-linux-gnu)
- GitHub Check: Build (armv7-linux-androideabi)
- GitHub Check: Build (aarch64-unknown-linux-gnu)
- GitHub Check: Build (aarch64-linux-android)
- GitHub Check: Test (x86_64-pc-windows-msvc)
- GitHub Check: Build (x86_64-apple-darwin)
- GitHub Check: Test (x86_64-apple-darwin)
- GitHub Check: Build (aarch64-apple-ios-sim)
- GitHub Check: Build (aarch64-pc-windows-msvc)
- GitHub Check: Build (aarch64-apple-darwin)
- GitHub Check: Test (x86_64-unknown-linux-gnu)
- GitHub Check: Build (aarch64-apple-ios)
- GitHub Check: Build (x86_64-pc-windows-msvc)
🔇 Additional comments (10)
livekit/src/room/participant/local_participant.rs (10)
219-222: No-op is appropriate here.
The hidden hook matches the local-participant semantics and keeps the API surface stable.
227-238: Accessor additions look consistent.
Getters mirror inner state clearly and keep ownership/copying predictable.Also applies to: 303-364
368-511: Docs + publish flow look solid.
The example-driven docs and publish flow are clear and helpful.
513-577: Unpublish + publication accessors look good.
The removal path and publication mapping are consistent.
579-623: Permission update helpers look fine.
The internal helpers are focused and straightforward.
633-644: RPC workflow structure looks good.
Handler registration and request/response plumbing are clear and cohesive.Also applies to: 655-913
1015-1034: publish_data mapping looks correct.
The packet translation and reliability handling are straightforward.
1039-1055: DTMF packet assembly looks fine.
The packet construction and reliable send path are consistent.
1064-1143: Deprecation block is clear.
Good separation of legacy APIs with explicit deprecation messaging.
1152-1181: Hidden FFI helpers look okay.
The internal methods are well isolated under#[doc(hidden)].
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai, your assessment that new methods have been added is incorrect. |
|
Tip For best results, initiate chat on the files or code changes.
The methods listed as "Method added" were not actually new methods being introduced to the API. Instead, as your PR description correctly states, this PR reorganizes existing methods in
The automated summary incorrectly interpreted the structural reorganization (moving methods between impl blocks) as additions to the public API, when in reality the public API surface remains the same - just better organized and documented. Thank you for the correction. I'll make sure to be more careful about distinguishing between actual API changes and organizational refactoring. |
Summary of changes:
LocalParticipantinto multiple impl blocks for better organization in generated docs.send_chat_message,edit_chat_message, andpublish_transcription.Preview docs locally with
cargo doc.