Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCall join/leave control flow was refactored to support cancellation of in-flight joins via a new JoinCanceledError. Internal coordination fields and cancellation-aware retry logic were added to Call.ts, tests for join/leave interactions were introduced, and small error-handling logging changes were applied to event handlers. A new re-export was added to the package entry. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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🧪 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 |
|
@oliverlaz post merge, could you merge this to callingx branch and handle the conflicts there as well please, would be helpful for us |
# Conflicts: # packages/client/src/Call.ts # packages/client/src/errors/SfuJoinError.ts # packages/client/src/errors/index.ts
# Conflicts: # packages/client/src/Call.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client/src/Call.ts`:
- Around line 926-932: Move the race-check so begin()/finish() remain balanced:
before calling this.joinController.begin() verify the current leaveRequestId vs
joinRequestLeaveId and call this.joinController.cancel()/throw if they differ;
this prevents begin() from being invoked when a concurrent leave already
occurred and avoids leaving the AbortController orphaned (adjust the check
around joinRequestLeaveId, this.leaveRequestId, and the
this.joinController.cancel()/new JoinCanceledError flow and keep the existing
this.joinController.setJoinTask and later finish() usage unchanged).
- Around line 907-911: watchCallAccepted currently awaits call.join() without
guarding against JoinCanceledError which can propagate out of Coordinator event
listeners; update the watchCallAccepted handler to wrap the await call.join() in
a try-catch that specifically handles JoinCanceledError (silence or no-op) and
rethrows or logs unexpected errors, and similarly audit watchCallRejected and
any other internal handlers that call call.join() to apply the same pattern;
additionally, modify StreamClient.dispatchEvent() to wrap each listener
invocation in a try-catch (mirroring Dispatcher.emit()) so listener errors
cannot escape the Coordinator dispatch path.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client/src/__tests__/Call.join.test.ts`:
- Around line 91-122: The test "stops join retries when leave is called while
retry loop is running" currently calls vi.useFakeTimers() and vi.useRealTimers()
but doesn't guarantee restoration if an assertion throws; wrap the test body
after vi.useFakeTimers() in a try/finally and call vi.useRealTimers() in the
finally block so fake timers are always restored even if createCall(),
call.join(), expectations on doJoinRequestMock, or call.leave() fail; this
touches the test function and references to vi.useFakeTimers, vi.useRealTimers,
createCall, call.join, and call.leave.
---
Duplicate comments:
In `@packages/client/src/Call.ts`:
- Around line 929-1003: watchCallAccepted (and any internal callers of
Call.join()) currently call await call.join() without handling the new
JoinCanceledError; update watchCallAccepted in
packages/client/src/events/call.ts and any other internal sites that invoke
Call.join() (search for ".join(" usage) to either catch JoinCanceledError and
treat it as a benign cancellation (e.g., no-op or early return) or
rethrow/propagate it if the caller expects cancellations, ensuring you only
swallow JoinCanceledError and not other errors; modify the surrounding async
function to wrap await call.join() in try/catch, check "err instanceof
JoinCanceledError" and handle accordingly, or add an explicit throws propagation
in the function signature where appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client/src/events/call.ts`:
- Around line 24-26: The catch on await call.join() currently logs every error
as a real failure; update it to distinguish JoinCanceledError from other errors
by importing JoinCanceledError from '../errors' and checking the error instance
in the catch handler (e.g., if (err instanceof JoinCanceledError) log at
debug/trace/info with a message indicating join was cancelled, else
call.logger.error('Failed to join call after call.accepted', err)); keep
references to call.join, call.leave, and call.logger.error when modifying the
handler.
3 participants
💡 Overview
Allows cancelling join retries through
call.leave().Up until now,
call.leave()was waiting for any ongoingcall.join()operations to finish successfully before disposing allocated resources.In cases where
join()couldn't successfully finish, and retries kick in, this meant that disposing of thecallinstance would be delayed until the retries are either exhausted or join succeeds.While this behavior makes sense to some extent, it clashes with the device's back navigation, as there was no way of interrupting the join flow.
This PR aims to solve that by allowing us to stop the retries at any time by invoking
call.leave().leave()still waits for any ongoingjoin()process to finish or reject.🎫 Ticket: https://linear.app/stream/issue/XYZ-123
📑 Docs: https://github.com/GetStream/docs-content/pull/
Summary by CodeRabbit
New Features
Bug Fixes
Tests