Conversation
📝 WalkthroughWalkthroughThe PR expands and reorganizes Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant LP as LocalParticipant
participant Net as Network/Signaling
participant Remote as Remote Participant/Handler
App->>LP: perform_rpc(data)
LP->>LP: publish_rpc_request()
LP->>Net: send RPC request
Net->>Remote: route request
Remote->>Remote: execute registered RPC method
Remote->>Net: send RPC response
Net->>LP: deliver response
LP->>LP: handle_incoming_rpc_response()
LP->>Net: publish_rpc_ack()
Net->>Remote: deliver ack
sequenceDiagram
participant App as Application
participant LP as LocalParticipant
participant StreamW as Stream Writer
participant DataPub as Data Publisher
participant Remote as Remote Participant
App->>LP: send_text(text, options)
LP->>StreamW: create TextStreamWriter
StreamW->>DataPub: publish_data(packet)
DataPub->>Remote: send via data channel
Remote-->>App: deliver text
App->>LP: stream_bytes(options)
LP->>StreamW: create ByteStreamWriter
StreamW->>DataPub: publish_raw_data(packet)
DataPub->>Remote: send bytes
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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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. |
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Assuming I'm not missing subtle logic changes intermixed in the diff (per one of my comments), this looks great, thanks for doing the refactoring!
| /// # options::TrackPublishOptions, | ||
| /// # webrtc::{prelude::*, audio_source::native::NativeAudioSource} | ||
| /// # }; | ||
| /// # async fn with_room(room: Room) -> RoomResult<()> { |
There was a problem hiding this comment.
question: Does with_room run automatically as part of the doctest? How does the runner know to do this? I would assume reading this at face value the doctest would be a no-op currently since that function is never run.
There was a problem hiding this comment.
with_room is a dummy function so the example can demonstrate working with an instance of Room without actually creating one. In this case, the doc test can only check syntax/types.
| /// Data packets. | ||
| /// | ||
| /// Low-level API for sending data between participants. | ||
| /// | ||
| /// LiveKit Docs: see [Data packets](https://docs.livekit.io/transport/data/packets/) | ||
| /// |
There was a problem hiding this comment.
All of these new docstrings are great, thank you for putting these in!
| /// Deprecated methods which will be removed in the next major release. | ||
| /// | ||
| /// Please use modern equivalents for new applications. | ||
| /// | ||
| #[deprecated = "Use data streams APIs instead"] | ||
| impl LocalParticipant { |
There was a problem hiding this comment.
question: Just to confirm - is it ok to officially deprecate these? I'm assuming a deprecation warning for these acts in a similar way to what a deprecation does in typescript (ie, some sort of editor info notice if one is using a fancy editor with LSP features and some sort of warning at build time), so all existing code will continue to work and this isn't a breaking change.
thought: It might be a good idea to link to the send_text method as part of the deprecation just to give people an actionable jumping off point when they see the deprecation.
thought: Also a related note - would it be worth doing a minor version bump for this rather than a patch version bump given the deprecations are somewhat of a public interface change that maybe wouldn't be ideal to happen when pulling in new patch releases? FWIW I don't have a strong opinion either way.
| @@ -343,16 +266,20 @@ impl LocalParticipant { | |||
| } | |||
| } | |||
|
|
|||
| pub async fn set_attributes(&self, attributes: HashMap<String, String>) -> RoomResult<()> { | |||
| pub fn metadata(&self) -> String { | |||
| self.inner.info.read().metadata.clone() | |||
| } | |||
|
|
|||
| pub async fn set_metadata(&self, metadata: String) -> RoomResult<()> { | |||
| if let Ok(response) = timeout(REQUEST_TIMEOUT, { | |||
| let request_id = self.inner.rtc_engine.session().signal_client().next_request_id(); | |||
| self.inner | |||
There was a problem hiding this comment.
(It is not 100% clear to me that there aren't minor modifications in all this refactoring due to the diff being fairly messy, but based on the PR description I am going to assume this is just reordering existing methods / no substantive logic changes have been made. If there are substantive changes you'd like a review of, please point them out!)
There was a problem hiding this comment.
There haven't been any changes to logic in this PR; just extensive rearranging of what was already there.
There was a problem hiding this comment.
Sounds good, thanks for confirming!
| /// Data streams: send data between participants reliably. | ||
| /// | ||
| /// LiveKit Docs: see [Sending text](https://docs.livekit.io/transport/data/text-streams/) and | ||
| /// [Sending files & bytes](https://docs.livekit.io/transport/data/byte-streams/) | ||
| /// | ||
| impl LocalParticipant { |
There was a problem hiding this comment.
suggestion(if-minor): it might be a good idea given this refactoring you have done to split up each of these impl blocks into its own file. That would mean that the implementation can be subdivided by feature type and a little less overwhelming to parse by eye.
ladvoc
force-pushed
the
ladvoc/doc-examples
branch
from
a2ff50f to
a747d66
Compare
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 (4)
livekit/src/room/participant/local_participant.rs (4)
519-539: Potential panics fromunwrap()calls.Lines 521-522 use
unwrap()onpublication.track()andtrack.transceiver(), which could panic if the track or transceiver isNone(e.g., due to a race condition or if the publication state is inconsistent).Suggested safer error handling
pub async fn unpublish_track( &self, sid: &TrackSid, // _stop_on_unpublish: bool, ) -> RoomResult<LocalTrackPublication> { let publication = self.remove_publication(sid); if let Some(TrackPublication::Local(publication)) = publication { - let track = publication.track().unwrap(); - let sender = track.transceiver().unwrap().sender(); + let track = publication.track().ok_or_else(|| { + RoomError::Internal("track already removed from publication".to_string()) + })?; + let transceiver = track.transceiver().ok_or_else(|| { + RoomError::Internal("transceiver not set on track".to_string()) + })?; + let sender = transceiver.sender(); self.inner.rtc_engine.remove_track(sender)?;
648-654: Potential panic if server version string is malformed.
Version::parse(&server_info.version).unwrap()will panic if the server returns a non-semver-compliant version string. Consider handling the parse error gracefully.Suggested fix
if !server_info.version.is_empty() { - 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)); + if let Ok(server_version) = Version::parse(&server_info.version) { + let min_required_version = Version::parse("1.8.0").unwrap(); + if server_version < min_required_version { + return Err(RpcError::built_in(RpcErrorCode::UnsupportedServer, None)); + } } }
700-701: Typo: "timout" should be "timeout".- // Wait for response timout + // Wait for response timeout
932-1005: Replaceself.session().unwrap()calls with proper error handling.All five data stream methods (
send_text,send_file,send_bytes,stream_text,stream_bytes) will panic if the session has been dropped. Since the session is stored as aWeak<RoomSession>, the.upgrade()call can fail if the owningRoomis disconnected or dropped.The current
StreamErrorenum does not have aNotConnectedor equivalent variant. Add a new error variant (e.g.,#[error("session not available")] SessionUnavailable) and useok_or_elseto return it instead of panicking.Suggested pattern
pub async fn send_text( &self, text: &str, options: StreamTextOptions, ) -> StreamResult<TextStreamInfo> { - self.session().unwrap().outgoing_stream_manager.send_text(text, options).await + let session = self.session().ok_or(StreamError::SessionUnavailable)?; + session.outgoing_stream_manager.send_text(text, options).await }Then add the variant to
StreamError:pub enum StreamError { #[error("stream has already been closed")] AlreadyClosed, + #[error("session not available")] + SessionUnavailable,
🤖 Fix all issues with AI agents
In `@livekit/src/room/participant/local_participant.rs`:
- Around line 1152-1162: The method publish_raw_data currently takes self by
value which consumes the LocalParticipant; change its signature to take &self
(i.e., pub async fn publish_raw_data(&self, packet: proto::DataPacket, reliable:
bool) -> RoomResult<()>) so it matches other publish methods and does not move
the participant, and ensure any call sites that passed ownership are updated to
call with a shared reference; the body can continue to call
self.inner.rtc_engine.publish_data(packet, kind, true).await.map_err(Into::into)
unchanged.
🧹 Nitpick comments (1)
livekit/src/room/participant/local_participant.rs (1)
1058-1144: Good deprecation strategy.The deprecated methods are properly grouped in a separate impl block with the
#[deprecated]attribute at the block level. This clearly signals to users that these methods should be migrated to data streams APIs.Minor style nit on line 1131: Rust allows field init shorthand when the variable name matches the field name.
let transcription_packet = proto::Transcription { transcribed_participant_identity: packet.participant_identity, - segments: segments, + segments, track_id: packet.track_id, };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
livekit/src/room/participant/local_participant.rs
🧰 Additional context used
🧬 Code graph analysis (1)
livekit/src/room/participant/local_participant.rs (6)
livekit/src/room/participant/mod.rs (2)
update_data_encryption_status(329-362)add_publication(387-450)livekit/src/room/participant/remote_participant.rs (9)
update_data_encryption_status(556-562)sid(492-494)identity(496-498)name(500-502)metadata(504-506)is_encrypted(551-553)kind(539-541)new(77-101)add_publication(332-464)livekit/src/room/publication/local.rs (5)
sid(101-103)name(105-107)kind(109-111)track(125-127)new(45-50)livekit/src/room/track/local_video_track.rs (5)
sid(67-69)name(71-73)kind(75-77)new(40-50)transceiver(138-140)livekit/src/room/track/local_audio_track.rs (5)
sid(67-69)name(71-73)kind(75-77)new(41-51)transceiver(138-140)livekit/src/room/participant/rpc.rs (2)
default(29-36)new(76-82)
⏰ 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 (aarch64-pc-windows-msvc)
- GitHub Check: Build (x86_64-apple-darwin)
- GitHub Check: Build (aarch64-linux-android)
- GitHub Check: Build (aarch64-unknown-linux-gnu)
- GitHub Check: Build (x86_64-pc-windows-msvc)
- GitHub Check: Build (aarch64-apple-ios)
- GitHub Check: Build (armv7-linux-androideabi)
- GitHub Check: Build (x86_64-unknown-linux-gnu)
- GitHub Check: Build (aarch64-apple-ios-sim)
- GitHub Check: Build (aarch64-apple-darwin)
- GitHub Check: Test (x86_64-unknown-linux-gnu)
- GitHub Check: Test (x86_64-pc-windows-msvc)
- GitHub Check: Test (x86_64-apple-darwin)
🔇 Additional comments (8)
livekit/src/room/participant/local_participant.rs (8)
51-90: LGTM!The type definitions and state structures are well-organized. The
RpcStateproperly separates pending acknowledgments, responses, and handlers. TheLocalInfostruct cleanly aggregates local participant state including encryption, RPC, and permissions.
92-140: LGTM!The
LocalParticipantstruct and its constructor are well-structured with proper Arc-based shared ownership for both the commonParticipantInnerand local-specificLocalInfo.
142-223: LGTM!The internal
pub(crate)methods are properly encapsulated. Theupdate_data_encryption_statusno-op is appropriately marked#[doc(hidden)]with a clear comment explaining that local participants don't receive data messages.
225-364: LGTM!The participant information accessors are cleanly organized. The setter methods (
set_name,set_metadata,set_attributes) follow a consistent pattern with proper timeout handling and error propagation. Each setter correctly preserves the other fields when sending theUpdateParticipantMetadatarequest.
368-438: Excellent documentation examples!The doc examples clearly demonstrate how to publish both audio and video tracks with appropriate configuration options. This significantly improves the developer experience.
542-623: LGTM!The track subscription permissions and publication management methods are properly implemented. The
unreachable!()macro usage inget_track_publicationandtrack_publicationsis acceptable sinceLocalParticipantshould only contain local track publications by design.
1008-1035: LGTM!The
publish_datamethod cleanly maps the high-levelDataPacketto the protocol-level representation with proper reliable/lossy handling.
1037-1056: LGTM!The telephony integration method
publish_dtmfis cleanly implemented for SIP DTMF signaling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| pub async fn publish_raw_data( | ||
| self, | ||
| packet: proto::DataPacket, | ||
| reliable: bool, | ||
| ) -> RoomResult<()> { | ||
| let kind = match reliable { | ||
| true => DataPacketKind::Reliable, | ||
| false => DataPacketKind::Lossy, | ||
| }; | ||
| self.inner.rtc_engine.publish_data(packet, kind, true).await.map_err(Into::into) | ||
| } |
There was a problem hiding this comment.
publish_raw_data takes self by value, consuming the participant.
The method signature uses self instead of &self, which moves ownership and consumes the LocalParticipant. This is likely unintended since calling this method would make the participant unusable afterward. Other publish methods use &self.
Suggested fix
pub async fn publish_raw_data(
- self,
+ &self,
packet: proto::DataPacket,
reliable: bool,
) -> RoomResult<()> {📝 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.
| pub async fn publish_raw_data( | |
| self, | |
| packet: proto::DataPacket, | |
| reliable: bool, | |
| ) -> RoomResult<()> { | |
| let kind = match reliable { | |
| true => DataPacketKind::Reliable, | |
| false => DataPacketKind::Lossy, | |
| }; | |
| self.inner.rtc_engine.publish_data(packet, kind, true).await.map_err(Into::into) | |
| } | |
| pub async fn publish_raw_data( | |
| &self, | |
| packet: proto::DataPacket, | |
| reliable: bool, | |
| ) -> RoomResult<()> { | |
| let kind = match reliable { | |
| true => DataPacketKind::Reliable, | |
| false => DataPacketKind::Lossy, | |
| }; | |
| self.inner.rtc_engine.publish_data(packet, kind, true).await.map_err(Into::into) | |
| } |
🤖 Prompt for AI Agents
In `@livekit/src/room/participant/local_participant.rs` around lines 1152 - 1162,
The method publish_raw_data currently takes self by value which consumes the
LocalParticipant; change its signature to take &self (i.e., pub async fn
publish_raw_data(&self, packet: proto::DataPacket, reliable: bool) ->
RoomResult<()>) so it matches other publish methods and does not move the
participant, and ensure any call sites that passed ownership are updated to call
with a shared reference; the body can continue to call
self.inner.rtc_engine.publish_data(packet, kind, true).await.map_err(Into::into)
unchanged.
2 participants
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.Summary by CodeRabbit
Release Notes
New Features
Deprecations
✏️ Tip: You can customize this high-level summary in your review settings.