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

Fix local echo in embedded mode #4498

Merged
merged 9 commits into from
Nov 14, 2024
Merged

Fix local echo in embedded mode #4498

merged 9 commits into from
Nov 14, 2024

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Nov 5, 2024

This fixes remote echos not appearing in the widget api. As a consequence, an event listener in a matroska widget will never receive events sent by itself withisSending() === false.

This is needed in the raise hand logic for element call. We send a raise hand and need to update the ui as soon as it is sent so that pressing the button again redacts the raise hand.

The issue is caused by the following:

  • An event gets a txId assigned by the embedded client (the widget)
  • This txId is tracked, so that the event can be matched once the remote echo is received. In that case the js-sdk "ignores" the remote event and instead updates the already existing local event to reflect it is send.
  • Using the widget api the event gets a txid twice. once in the widget (but that txid does not get forwarded to the client through the widget api) and once while it is sent by the client.
  • The event that then gets to the widget uses a different txid. The two events cannot be matched up because of the different txid's. since we do have the same evId however we still ignore the remote echo.
  • We end up with an ignored remote echo that did not update the local event to be sucessfully sent.

The fix works as following:

  • We track the txId of the local event.
  • We block all remote echos until we get a response from the send action.
  • Once we have the response form the send action we know the txId - evId relation
  • We take the remote echo and update the txId to not be the one created by the cleint but by the widget matching the local event of the widget.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

src/embedded.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@florianduros florianduros left a comment

Choose a reason for hiding this comment

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

What issue this PR aims to fix?

@toger5
Copy link
Contributor Author

toger5 commented Nov 6, 2024

@florianduros I updated the description to reflect what the PR fixes.

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Can't we just have the other side respect the txn_id we choose instead of having to track the possibility of it changing?

src/embedded.ts Outdated Show resolved Hide resolved
@toger5
Copy link
Contributor Author

toger5 commented Nov 6, 2024

Can't we just have the other side respect the txn_id we choose instead of having to track the possibility of it changing?

This was discussed but we concluded, that the widget should not deal with the txId selection.

  • To keep the api simple
  • The requirement comes from the js-sdk local echo logic which most widgets do not even use
  • It would require us to update the web and rust sdk driver.

But yes that is also an option.

@t3chguy
Copy link
Member

t3chguy commented Nov 7, 2024

Please avoid force pushing once you have received review as it discards any review state and means I have to review the entire change rather than just the changes.

src/embedded.ts Outdated Show resolved Hide resolved
robintown added a commit to robintown/element-call that referenced this pull request Nov 9, 2024
This bumps matrix-js-sdk to a preview branch that includes matrix-org/matrix-js-sdk#4498 and matrix-org/matrix-js-sdk#4494, and matrix-widget-api to 1.10.0.
@toger5
Copy link
Contributor Author

toger5 commented Nov 14, 2024

We do not squash + merge right?
So I guess it makes sense to squash all the changes before I add it to the queue?

@t3chguy
Copy link
Member

t3chguy commented Nov 14, 2024

The merge queue only uses squash merges

@toger5
Copy link
Contributor Author

toger5 commented Nov 14, 2024

Oh we are missing an EC reviewer anyways.

@florianduros florianduros removed their request for review November 14, 2024 12:27
@toger5 toger5 added this pull request to the merge queue Nov 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 14, 2024
@toger5 toger5 added this pull request to the merge queue Nov 14, 2024
Merged via the queue into develop with commit 325dace Nov 14, 2024
28 checks passed
@toger5 toger5 deleted the toger5/local-echo branch November 14, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants