Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Use disptacher to answer/reject calls in call tiles#6705

Merged
dbkr merged 2 commits into
matrix-org:developfrom
SimonBrandner:fix/call-tile-dispatch/18825
Sep 7, 2021
Merged

Use disptacher to answer/reject calls in call tiles#6705
dbkr merged 2 commits into
matrix-org:developfrom
SimonBrandner:fix/call-tile-dispatch/18825

Conversation

@SimonBrandner
Copy link
Copy Markdown
Contributor

@SimonBrandner SimonBrandner commented Aug 28, 2021

Fixes element-hq/element-web#18825
Type: task


This change is marked as an internal change (Task), so will not be included in the changelog.

Preview: https://6130f3e40a73a5fcda261486--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner requested a review from a team as a code owner August 28, 2021 13:07
@github-actions github-actions Bot added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Aug 28, 2021
Copy link
Copy Markdown
Contributor

@Palid Palid left a comment

Choose a reason for hiding this comment

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

LGTM, the two comments are a minor thing, but it wouldn't hurt to address them!

Comment thread src/components/structures/CallEventGrouper.ts Outdated
}

private get roomId(): string {
return [...this.events][0]?.getRoomId();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we always 100% certain we have at least one element in this.events? Seems from my short code analysis that this case should never happen, but could we add an additional check for it, just to be sure? Same would go for callId. Otherwise LGTM.

Copy link
Copy Markdown
Contributor Author

@SimonBrandner SimonBrandner Sep 2, 2021

Choose a reason for hiding this comment

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

We should always have at least one event, but I've added some ? and made the return type into string | undefined, hopefully, it's better this way?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We should also probably have some null-checks though those should be in the CallHandler. I am going to do some more refactoring in that area, so this is one of the things I will look into

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner requested a review from Palid September 2, 2021 15:54
@SimonBrandner SimonBrandner added the X-Release-Blocker This affects the current release cycle and must be solved for a release to happen label Sep 3, 2021
@SimonBrandner
Copy link
Copy Markdown
Contributor Author

Blocker so that #6691 isn't half-useless

@dbkr dbkr merged commit 495b6dc into matrix-org:develop Sep 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

T-Task Refactoring, enabling or disabling functionality, other engineering tasks X-Release-Blocker This affects the current release cycle and must be solved for a release to happen

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CallEventGrouper should use dispatcher to answer/reject calls

3 participants