Skip to content

Conversation

@GuuJiang
Copy link
Contributor

@GuuJiang GuuJiang commented Jan 3, 2026

fix for ISSUE-823

@davidzhao davidzhao requested a review from Copilot January 4, 2026 00:13
Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work! just a couple of nits and I think we can merge this one.


message ParticipantPermissionChanged {
required string participant_identity = 1;
optional ParticipantPermission permission = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be optional? seems like it should always be there

},
ParticipantPermissionChanged {
participant: Participant,
permission: Option<proto::ParticipantPermission>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same concern here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same concerns from the beginning and have been struggling with this decision.

The root cause is that the protocol layer uses proto3 syntax, which makes the generated nested structs optional by default. Even though we know in practice it will never be empty (and we can get the semantic default value even if it's empty in the proto3 world), after careful consideration, I decided to make the permission field optional in the ParticipantInfo struct in the FFI layer to avoid potential breaking changes.

I completely agree that it should be required in the ParticipantPermissionChanged event. However, I initially thought it might be better to keep consistency with the ParticipantInfo struct. I'm happy to change it to required if you think that's the better approach.

BTW, I made a new commit to initialize the permission field(It should be discovered earlier if choose required instead, LOL)

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR exposes participant permissions to the FFI (Foreign Function Interface) layer to fix issue #823. The change enables FFI consumers to access and monitor permission changes for both local and remote participants.

Key changes:

  • Added permission field to ParticipantInfo struct and exposed it via a public getter method
  • Implemented ParticipantPermissionChanged event that fires when participant permissions are updated
  • Added FFI protocol definitions and conversion logic to bridge Rust types to the FFI layer

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
livekit/src/room/participant/mod.rs Added permission field to ParticipantInfo, permission change event handler, and update logic to detect and dispatch permission changes
livekit/src/room/participant/remote_participant.rs Added permission getter method and permission change event handler for remote participants
livekit/src/room/participant/local_participant.rs Added permission getter method and permission change event handler for local participants
livekit/src/room/mod.rs Added ParticipantPermissionChanged room event and registered event handlers for both local and remote participants
livekit-ffi/src/server/room.rs Added event forwarding logic to convert Rust ParticipantPermissionChanged events to FFI protocol messages
livekit-ffi/src/conversion/participant.rs Implemented conversion from livekit_proto::ParticipantPermission to FFI proto::ParticipantPermission
livekit-ffi/protocol/participant.proto Defined ParticipantPermission message structure with proper field mappings and added import for TrackSource enum
livekit-ffi/protocol/room.proto Added ParticipantPermissionChanged message to RoomEvent oneof

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants