fix: resolve regressions across media and signaling#66
Conversation
- focusing on media pipeline reliability - WebRTC renegotiation, and - robust state transitions between the lobby and meeting room. Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWhen a host admits a user from the lobby, the service now retrieves the lobby entry to extract the user's ChangesUser Admission and Join Broadcast
Sequence Diagram(s)sequenceDiagram
participant Client
participant MeetingService
participant MeetingStateService
participant ConnectionManager
participant ExistingParticipants
Client->>MeetingService: admit_user(host_id, target_user_id)
MeetingService->>MeetingStateService: get_lobby()
MeetingStateService-->>MeetingService: lobby_state with display_name
MeetingService->>MeetingStateService: admit_from_lobby(target_user_id)
MeetingStateService->>MeetingStateService: read display_name, set role=guest
MeetingService->>ConnectionManager: broadcast user_joined
ConnectionManager->>ExistingParticipants: user_joined(user_id, display_name, role)
MeetingService-->>Client: admission_success
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (1)
tests/meeting/test_meeting_service.py (1)
561-575: ⚡ Quick winAssert the new signaling side effects in the happy-path admit test.
This test now seeds lobby metadata, but it still doesn’t verify the new
send_to_user/broadcast_to_roomcontract that this PR introduces.✅ Suggested assertion additions
- async def test_host_admits_user_from_lobby(self) -> None: + async def test_host_admits_user_from_lobby(self, mock_cm: MagicMock) -> None: svc, repo, state = _build_service() host = _make_user() room = _make_room(host_id=host.id) @@ await svc.admit_user(host=host, room_code="ABCDEF123456", target_user_id="u99") + state.get_lobby.assert_awaited_once_with("ABCDEF123456") state.admit_from_lobby.assert_awaited_once_with("ABCDEF123456", "u99") + mock_cm.send_to_user.assert_awaited_once_with( + "ABCDEF123456", "u99", {"type": "admitted", "room_code": "ABCDEF123456"} + ) + mock_cm.broadcast_to_room.assert_awaited_once_with( + "ABCDEF123456", + { + "type": "user_joined", + "user_id": "u99", + "display_name": "Guest Bob", + "role": "guest", + }, + sender_id="u99", + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/meeting/test_meeting_service.py` around lines 561 - 575, The test_host_admits_user_from_lobby happy-path test seeds lobby metadata but doesn't assert the new signaling side effects; after calling svc.admit_user, add assertions that svc.signaling.send_to_user was awaited with the admitted user's id (target_user_id/"u99") and the expected payload, and that svc.signaling.broadcast_to_room was awaited with the room code ("ABCDEF123456") and the expected room-level broadcast payload; reference the test function test_host_admits_user_from_lobby, the service method svc.admit_user, and the mock methods svc.signaling.send_to_user and svc.signaling.broadcast_to_room when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/modules/meeting/service.py`:
- Line 502: Two comments exceed the 88-character line limit and are causing Ruff
E501 failures; locate the comment string "# Fetch display_name BEFORE
admit_from_lobby removes the entry from the lobby hash." and the other long line
around the same area (near line 526) in app/modules/meeting/service.py and
reflow them to be under 88 characters by splitting into two shorter comment
lines or shortening wording (e.g., "# Fetch display_name BEFORE
admit_from_lobby" on one line and "# removes the entry from the lobby hash." on
the next), ensuring no code semantics change and keeping the original meaning.
---
Nitpick comments:
In `@tests/meeting/test_meeting_service.py`:
- Around line 561-575: The test_host_admits_user_from_lobby happy-path test
seeds lobby metadata but doesn't assert the new signaling side effects; after
calling svc.admit_user, add assertions that svc.signaling.send_to_user was
awaited with the admitted user's id (target_user_id/"u99") and the expected
payload, and that svc.signaling.broadcast_to_room was awaited with the room code
("ABCDEF123456") and the expected room-level broadcast payload; reference the
test function test_host_admits_user_from_lobby, the service method
svc.admit_user, and the mock methods svc.signaling.send_to_user and
svc.signaling.broadcast_to_room when adding these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 183a921f-d969-46a7-8f05-6388972ef94c
📒 Files selected for processing (3)
app/modules/meeting/service.pyapp/modules/meeting/state.pytests/meeting/test_meeting_service.py
Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
1 participant
Summary by CodeRabbit
New Features
Tests