Skip to content

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Sep 19, 2025

We need a way to cap the lifetime of the m.rtc.notification event. It makes sense to combine this with a method that also validates all the fields of the event.
parseCallNotificationContent does the validation and tha cap.
Signed-off-by: Timo K [email protected]

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).

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Seems reasonable, can we get some tests, specifically on the cap value

Signed-off-by: Timo K <[email protected]>
Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

Some questions

* @returns a parsed IRTCNotificationContent
*/
export function parseCallNotificationContent(content: IContent): IRTCNotificationContent {
if (!content["m.mentions"]) {
Copy link
Member

Choose a reason for hiding this comment

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

so this is expected to at least be an empty [] array? cannot be skipped? I think ruma skips it if it is not set

Copy link
Contributor Author

@toger5 toger5 Sep 19, 2025

Choose a reason for hiding this comment

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

I think it also might already do checks on this in the notifier. Lets only check for it to be an obj here.

throw new Error("Missing or invalid lifetime");
}

if (content["decline_reason"] && typeof content["decline_reason"] !== "string") {
Copy link
Member

Choose a reason for hiding this comment

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

This is new to me, a decline_reason in a RTCNotificationContent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right this is legacy... Not there anymore. its from the time where we used the notification event as the decline as well. This should have been done in the PR that added the decline event type...

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.

3 participants