Skip to content
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

CF SDK - Implicit reattach #1110

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

CF SDK - Implicit reattach #1110

wants to merge 18 commits into from

Conversation

iAmmar7
Copy link
Collaborator

@iAmmar7 iAmmar7 commented Aug 28, 2024

Description

This PR will enable developers to integrate call reattachment without explicitly calling the "reattach" method. The "dial" method now tracks calls based on the destination address, and in cases where the call is not terminated correctly (e.g., due to a page reload, or the user pressing the browser back button), the "dial" method will automatically perform an implicit reattach without notifying the developer or user.

This is the default behavior which can be overridden by passing the optional flag allowReattach: false to the "dial" method.

The explicit "reattach" method is still available for the developer to use.

Type of change

  • Internal refactoring
  • Bug fix (bugfix - non-breaking)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code snippets

const call = await client.dial({ to: "......", allowReattach: true|false })

Copy link

changeset-bot bot commented Aug 28, 2024

🦋 Changeset detected

Latest commit: 86abe0b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@signalwire/webrtc Minor
@signalwire/js Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@iAmmar7 iAmmar7 changed the title WIP: CF SDK - Implicit reattach CF SDK - Implicit reattach Aug 29, 2024
@iAmmar7 iAmmar7 changed the title CF SDK - Implicit reattach WIP: CF SDK - Implicit reattach Aug 29, 2024
@iAmmar7 iAmmar7 changed the title WIP: CF SDK - Implicit reattach CF SDK - Implicit reattach Sep 2, 2024
ayeminag
ayeminag previously approved these changes Sep 2, 2024
let key = sessionStorage.key(i)

// If the key starts with "ci-" and is not the current key, remove it
if (key?.startsWith('ci-') && key !== callIdKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we safely remove the other keys here?

What if the user is using a second tab to call another address, but then it needs to "reattach" on the first tab?

This is why, IMO, the SDK should not try to rule out whether the dial request should be a reattach or a plain dial. There are too many Application concerns to be considered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regarding the second tab, we are using session storage, which means that two different tabs will have entirely separate storage. All these features work during one tab session.

I added this clear storage feature in my last few commits. The reasoning behind this was that after disconnecting from a call if the user joins a different call and then later rejoins the first call, we might not want to automatically reattach.

I have no strong objection to this feature, though; we can remove it so that the user is always reattached if they did not properly detach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes sense and, in this case, is safe.
However, I do question whether we should clean it, particularly if we're not taking into account the app use case.
I'm fine with both ways if the developer can change the behavior to match the app's needs.

audio: params.audio ?? true,
video: params.audio ?? true,
negotiateAudio: params.negotiateAudio ?? true,
negotiateVideo: params.negotiateVideo ?? true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this going to force video negotiation by default, even when the call is audio-only?

We need to consolidate the makeCallFabricObject default so we don't have to worry about changing or fixing it in 2 places.

Are most of the defaults distinct for inbound versus outbound?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In an outbound call, we make this decision based on the to param which is the destination address. In the case of the inbound call, there is no to param exists.

Copy link
Collaborator

@jpsantosbh jpsantosbh left a comment

Choose a reason for hiding this comment

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

look good so far we just need to solve the cleanup issue.
and check with product if we should make the the Reattach Feature implicit.

@iAmmar7 iAmmar7 requested a review from jpsantosbh September 3, 2024 14:09
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.

4 participants