From f0d2c191849ed0154c3ac57418a624fb997d4d71 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Mon, 9 Jan 2023 13:50:03 +0100 Subject: [PATCH 01/32] allow an explicit error value again in ErrorBoundary --- src/utils/ErrorBoundary.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/utils/ErrorBoundary.ts b/src/utils/ErrorBoundary.ts index 520d2a35..d13065f9 100644 --- a/src/utils/ErrorBoundary.ts +++ b/src/utils/ErrorBoundary.ts @@ -25,22 +25,22 @@ export class ErrorBoundary { * Executes callback() and then runs errorCallback() on error. * This will never throw but instead return `errorValue` if an error occured. */ - try(callback: () => T): T | typeof ErrorValue; - try(callback: () => Promise): Promise | typeof ErrorValue { + try(callback: () => T, errorValue?: E): T | typeof errorValue; + try(callback: () => Promise, errorValue?: E): Promise | typeof errorValue { try { - let result: T | Promise = callback(); + let result: T | Promise = callback(); if (result instanceof Promise) { result = result.catch(err => { this._error = err; this.errorCallback(err); - return ErrorValue; + return errorValue; }); } return result; } catch (err) { this._error = err; this.errorCallback(err); - return ErrorValue; + return errorValue; } } @@ -56,9 +56,9 @@ export function tests() { const boundary = new ErrorBoundary(() => emitted = true); const result = boundary.try(() => { throw new Error("fail!"); - }); + }, 0); assert(emitted); - assert.strictEqual(result, ErrorValue); + assert.strictEqual(result, 0); }, "return value of callback is forwarded": assert => { let emitted = false; @@ -74,9 +74,9 @@ export function tests() { const boundary = new ErrorBoundary(() => emitted = true); const result = await boundary.try(async () => { throw new Error("fail!"); - }); + }, 0); assert(emitted); - assert.strictEqual(result, ErrorValue); + assert.strictEqual(result, 0); } } } \ No newline at end of file From b1687d71155ffaf50a4b1ed541c2bd57d7a16602 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Mon, 9 Jan 2023 13:50:27 +0100 Subject: [PATCH 02/32] introduce error boundary in call member --- src/matrix/calls/group/Member.ts | 185 +++++++++++++++++-------------- 1 file changed, 103 insertions(+), 82 deletions(-) diff --git a/src/matrix/calls/group/Member.ts b/src/matrix/calls/group/Member.ts index 9debef22..4ceaf46f 100644 --- a/src/matrix/calls/group/Member.ts +++ b/src/matrix/calls/group/Member.ts @@ -19,6 +19,7 @@ import {makeTxnId, makeId} from "../../common"; import {EventType, CallErrorCode} from "../callEventTypes"; import {formatToDeviceMessagesPayload} from "../../common"; import {sortedIndex} from "../../../utils/sortedIndex"; +import { ErrorBoundary } from "../../../utils/ErrorBoundary"; import type {MuteSettings} from "../common"; import type {Options as PeerCallOptions, RemoteMedia} from "../PeerCall"; @@ -94,6 +95,9 @@ class MemberConnection { export class Member { private connection?: MemberConnection; private expireTimeout?: Timeout; + private errorBoundary = new ErrorBoundary(err => { + this.options.emitUpdate(this, "error"); + }); constructor( public member: RoomMember, @@ -104,6 +108,10 @@ export class Member { this._renewExpireTimeout(updateMemberLog); } + get error(): Error | undefined { + return this.errorBoundary.error; + } + private _renewExpireTimeout(log: ILogItem) { this.expireTimeout?.dispose(); this.expireTimeout = undefined; @@ -166,23 +174,26 @@ export class Member { /** @internal */ connect(localMedia: LocalMedia, localMuteSettings: MuteSettings, turnServer: BaseObservableValue, memberLogItem: ILogItem): ILogItem | undefined { - if (this.connection) { - return; - } - // Safari can't send a MediaStream to multiple sources, so clone it - const connection = new MemberConnection( - localMedia.clone(), - localMuteSettings, - turnServer, - memberLogItem - ); - this.connection = connection; - let connectLogItem; - connection.logItem.wrap("connect", async log => { - connectLogItem = log; - await this.callIfNeeded(log); + return this.errorBoundary.try(() => { + if (this.connection) { + return; + } + // Safari can't send a MediaStream to multiple sources, so clone it + const connection = new MemberConnection( + localMedia.clone(), + localMuteSettings, + turnServer, + memberLogItem + ); + this.connection = connection; + let connectLogItem: ILogItem | undefined; + connection.logItem.wrap("connect", async log => { + connectLogItem = log; + await this.callIfNeeded(log); + }); + throw new Error("connect failed!"); + return connectLogItem; }); - return connectLogItem; } private callIfNeeded(log: ILogItem): Promise { @@ -211,30 +222,34 @@ export class Member { /** @internal */ disconnect(hangup: boolean): ILogItem | undefined { - const {connection} = this; - if (!connection) { - return; - } - let disconnectLogItem; - // if if not sending the hangup, still log disconnect - connection.logItem.wrap("disconnect", async log => { - disconnectLogItem = log; - if (hangup && connection.peerCall) { - await connection.peerCall.hangup(CallErrorCode.UserHangup, log); + return this.errorBoundary.try(() => { + const {connection} = this; + if (!connection) { + return; } + let disconnectLogItem; + // if if not sending the hangup, still log disconnect + connection.logItem.wrap("disconnect", async log => { + disconnectLogItem = log; + if (hangup && connection.peerCall) { + await connection.peerCall.hangup(CallErrorCode.UserHangup, log); + } + }); + connection.dispose(); + this.connection = undefined; + return disconnectLogItem; }); - connection.dispose(); - this.connection = undefined; - return disconnectLogItem; } /** @internal */ updateCallInfo(callDeviceMembership: CallDeviceMembership, causeItem: ILogItem) { - this.callDeviceMembership = callDeviceMembership; - this._renewExpireTimeout(causeItem); - if (this.connection) { - this.connection.logItem.refDetached(causeItem); - } + this.errorBoundary.try(() => { + this.callDeviceMembership = callDeviceMembership; + this._renewExpireTimeout(causeItem); + if (this.connection) { + this.connection.logItem.refDetached(causeItem); + } + }); } /** @internal */ @@ -308,49 +323,51 @@ export class Member { /** @internal */ handleDeviceMessage(message: SignallingMessage, syncLog: ILogItem): void { - const {connection} = this; - if (connection) { - const destSessionId = message.content.dest_session_id; - if (destSessionId !== this.options.sessionId) { - const logItem = connection.logItem.log({l: "ignoring to_device event with wrong session_id", destSessionId, type: message.type}); - syncLog.refDetached(logItem); - return; - } - // if there is no peerCall, we either create it with an invite and Handle is implied or we'll ignore it - let action = IncomingMessageAction.Handle; - if (connection.peerCall) { - action = connection.peerCall.getMessageAction(message); - // deal with glare and replacing the call before creating new calls - if (action === IncomingMessageAction.InviteGlare) { - const {shouldReplace, log} = connection.peerCall.handleInviteGlare(message, this.deviceId, connection.logItem); - if (log) { - syncLog.refDetached(log); - } - if (shouldReplace) { - connection.peerCall = undefined; - action = IncomingMessageAction.Handle; - } + this.errorBoundary.try(() => { + const {connection} = this; + if (connection) { + const destSessionId = message.content.dest_session_id; + if (destSessionId !== this.options.sessionId) { + const logItem = connection.logItem.log({l: "ignoring to_device event with wrong session_id", destSessionId, type: message.type}); + syncLog.refDetached(logItem); + return; } - } - if (message.type === EventType.Invite && !connection.peerCall) { - connection.peerCall = this._createPeerCall(message.content.call_id); - } - if (action === IncomingMessageAction.Handle) { - const idx = sortedIndex(connection.queuedSignallingMessages, message, (a, b) => a.content.seq - b.content.seq); - connection.queuedSignallingMessages.splice(idx, 0, message); + // if there is no peerCall, we either create it with an invite and Handle is implied or we'll ignore it + let action = IncomingMessageAction.Handle; if (connection.peerCall) { - const hasNewMessageBeenDequeued = this.dequeueSignallingMessages(connection, connection.peerCall, message, syncLog); - if (!hasNewMessageBeenDequeued) { - syncLog.refDetached(connection.logItem.log({l: "queued signalling message", type: message.type, seq: message.content.seq})); + action = connection.peerCall.getMessageAction(message); + // deal with glare and replacing the call before creating new calls + if (action === IncomingMessageAction.InviteGlare) { + const {shouldReplace, log} = connection.peerCall.handleInviteGlare(message, this.deviceId, connection.logItem); + if (log) { + syncLog.refDetached(log); + } + if (shouldReplace) { + connection.peerCall = undefined; + action = IncomingMessageAction.Handle; + } } } - } else if (action === IncomingMessageAction.Ignore && connection.peerCall) { - const logItem = connection.logItem.log({l: "ignoring to_device event with wrong call_id", callId: message.content.call_id, type: message.type}); - syncLog.refDetached(logItem); + if (message.type === EventType.Invite && !connection.peerCall) { + connection.peerCall = this._createPeerCall(message.content.call_id); + } + if (action === IncomingMessageAction.Handle) { + const idx = sortedIndex(connection.queuedSignallingMessages, message, (a, b) => a.content.seq - b.content.seq); + connection.queuedSignallingMessages.splice(idx, 0, message); + if (connection.peerCall) { + const hasNewMessageBeenDequeued = this.dequeueSignallingMessages(connection, connection.peerCall, message, syncLog); + if (!hasNewMessageBeenDequeued) { + syncLog.refDetached(connection.logItem.log({l: "queued signalling message", type: message.type, seq: message.content.seq})); + } + } + } else if (action === IncomingMessageAction.Ignore && connection.peerCall) { + const logItem = connection.logItem.log({l: "ignoring to_device event with wrong call_id", callId: message.content.call_id, type: message.type}); + syncLog.refDetached(logItem); + } + } else { + syncLog.log({l: "member not connected", userId: this.userId, deviceId: this.deviceId}); } - } else { - syncLog.log({l: "member not connected", userId: this.userId, deviceId: this.deviceId}); - } + }); } private dequeueSignallingMessages(connection: MemberConnection, peerCall: PeerCall, newMessage: SignallingMessage, syncLog: ILogItem): boolean { @@ -373,19 +390,23 @@ export class Member { /** @internal */ async setMedia(localMedia: LocalMedia, previousMedia: LocalMedia): Promise { - const {connection} = this; - if (connection) { - connection.localMedia = localMedia.replaceClone(connection.localMedia, previousMedia); - await connection.peerCall?.setMedia(connection.localMedia, connection.logItem); - } + return this.errorBoundary.try(async () => { + const {connection} = this; + if (connection) { + connection.localMedia = localMedia.replaceClone(connection.localMedia, previousMedia); + await connection.peerCall?.setMedia(connection.localMedia, connection.logItem); + } + }); } async setMuted(muteSettings: MuteSettings): Promise { - const {connection} = this; - if (connection) { - connection.localMuteSettings = muteSettings; - await connection.peerCall?.setMuted(muteSettings, connection.logItem); - } + return this.errorBoundary.try(async () => { + const {connection} = this; + if (connection) { + connection.localMuteSettings = muteSettings; + await connection.peerCall?.setMuted(muteSettings, connection.logItem); + } + }); } private _createPeerCall(callId: string): PeerCall { From 7f9edbb742bb78b6a782bce5f0670a46e4bcc3f5 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Mon, 9 Jan 2023 14:03:14 +0100 Subject: [PATCH 03/32] this can be private --- src/matrix/calls/group/GroupCall.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/matrix/calls/group/GroupCall.ts b/src/matrix/calls/group/GroupCall.ts index cda5a40f..01ff4d31 100644 --- a/src/matrix/calls/group/GroupCall.ts +++ b/src/matrix/calls/group/GroupCall.ts @@ -276,7 +276,7 @@ export class GroupCall extends EventEmitter<{change: never}> { }); } - terminate(log?: ILogItem): Promise { + private terminate(log?: ILogItem): Promise { return this.options.logger.wrapOrRun(log, {l: "terminate call", t: CALL_LOG_TYPE}, async log => { if (this._state === GroupCallState.Fledgling) { return; From 1e4180a71ff4f4ef5218eae861035a4bc5ca6387 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Mon, 9 Jan 2023 14:03:52 +0100 Subject: [PATCH 04/32] add error boundary to GroupCall --- src/matrix/calls/group/GroupCall.ts | 321 +++++++++++++++------------- 1 file changed, 177 insertions(+), 144 deletions(-) diff --git a/src/matrix/calls/group/GroupCall.ts b/src/matrix/calls/group/GroupCall.ts index 01ff4d31..53a87d5f 100644 --- a/src/matrix/calls/group/GroupCall.ts +++ b/src/matrix/calls/group/GroupCall.ts @@ -21,6 +21,7 @@ import {MuteSettings, CALL_LOG_TYPE, CALL_MEMBER_VALIDITY_PERIOD_MS, mute} from import {MemberChange, RoomMember} from "../../room/members/RoomMember"; import {EventEmitter} from "../../../utils/EventEmitter"; import {EventType, CallIntent} from "../callEventTypes"; +import { ErrorBoundary } from "../../../utils/ErrorBoundary"; import type {Options as MemberOptions} from "./Member"; import type {TurnServerSource} from "../TurnServerSource"; @@ -92,6 +93,9 @@ export class GroupCall extends EventEmitter<{change: never}> { private bufferedDeviceMessages = new Map>>(); /** Set between calling join and leave. */ private joinedData?: JoinedData; + private errorBoundary = new ErrorBoundary(err => { + this.emitChange(); + }); constructor( public readonly id: string, @@ -154,6 +158,10 @@ export class GroupCall extends EventEmitter<{change: never}> { return this.joinedData?.logItem; } + get error(): Error | undefined { + return this.errorBoundary.error; + } + async join(localMedia: LocalMedia): Promise { if (this._state !== GroupCallState.Created || this.joinedData) { return; @@ -206,6 +214,9 @@ export class GroupCall extends EventEmitter<{change: never}> { // and update the track info so PeerCall can use it to send up to date metadata, this.joinedData.localMuteSettings.updateTrackInfo(localMedia.userMedia); this.emitChange(); //allow listeners to see new media/mute settings + // TODO: if setMedia fails on one of the members, we should revert to the old media + // on the members processed so far, and show an error that we could not set the new media + // for this, we will need to remove the usage of the errorBoundary in member.setMedia. await Promise.all(Array.from(this._members.values()).map(m => { return m.setMedia(localMedia, oldMedia); })); @@ -234,6 +245,9 @@ export class GroupCall extends EventEmitter<{change: never}> { if (this.localMedia) { mute(this.localMedia, muteSettings, this.joinedData!.logItem); } + // TODO: if setMuted fails on one of the members, we should revert to the old media + // on the members processed so far, and show an error that we could not set the new media + // for this, we will need to remove the usage of the errorBoundary in member.setMuted. await Promise.all(Array.from(this._members.values()).map(m => { return m.setMuted(joinedData.localMuteSettings); })); @@ -271,7 +285,14 @@ export class GroupCall extends EventEmitter<{change: never}> { log.set("already_left", true); } } finally { - this.disconnect(log); + // disconnect is called both from the sync loop and from methods like this one that + // are called from the view model. We want errors during the sync loop being caught + // by the errorboundary, but since leave is called from the view model, we want + // the error to be thrown. So here we check if disconnect succeeded, and if not + // we rethrow the error put into the errorBoundary. + if(!this.disconnect(log)) { + throw this.errorBoundary.error; + } } }); } @@ -308,126 +329,134 @@ export class GroupCall extends EventEmitter<{change: never}> { /** @internal */ updateCallEvent(callContent: Record, syncLog: ILogItem) { - syncLog.wrap({l: "update call", t: CALL_LOG_TYPE, id: this.id}, log => { - this.callContent = callContent; - if (this._state === GroupCallState.Creating) { - this._state = GroupCallState.Created; - } - log.set("status", this._state); - this.emitChange(); + this.errorBoundary.try(() => { + syncLog.wrap({l: "update call", t: CALL_LOG_TYPE, id: this.id}, log => { + this.callContent = callContent; + if (this._state === GroupCallState.Creating) { + this._state = GroupCallState.Created; + } + log.set("status", this._state); + this.emitChange(); + }); }); } /** @internal */ updateRoomMembers(memberChanges: Map) { - for (const change of memberChanges.values()) { - const {member} = change; - for (const callMember of this._members.values()) { - // find all call members for a room member (can be multiple, for every device) - if (callMember.userId === member.userId) { - callMember.updateRoomMember(member); + this.errorBoundary.try(() => { + for (const change of memberChanges.values()) { + const {member} = change; + for (const callMember of this._members.values()) { + // find all call members for a room member (can be multiple, for every device) + if (callMember.userId === member.userId) { + callMember.updateRoomMember(member); + } } } - } - } - - /** @internal */ - updateMembership(userId: string, roomMember: RoomMember, callMembership: CallMembership, syncLog: ILogItem) { - syncLog.wrap({l: "update call membership", t: CALL_LOG_TYPE, id: this.id, userId}, log => { - const now = this.options.clock.now(); - const devices = callMembership["m.devices"]; - const previousDeviceIds = this.getDeviceIdsForUserId(userId); - for (const device of devices) { - const deviceId = device.device_id; - const memberKey = getMemberKey(userId, deviceId); - if (userId === this.options.ownUserId && deviceId === this.options.ownDeviceId) { - log.wrap("update own membership", log => { - if (this.hasJoined) { - if (this.joinedData) { - this.joinedData.logItem.refDetached(log); - } - this._setupRenewMembershipTimeout(device, log); - } - if (this._state === GroupCallState.Joining) { - log.set("joined", true); - this._state = GroupCallState.Joined; - this.emitChange(); - } - }); - } else { - log.wrap({l: "update device membership", id: memberKey, sessionId: device.session_id}, log => { - if (isMemberExpired(device, now)) { - log.set("expired", true); - const member = this._members.get(memberKey); - if (member) { - member.dispose(); - this._members.remove(memberKey); - log.set("removed", true); - } - return; - } - let member = this._members.get(memberKey); - const sessionIdChanged = member && member.sessionId !== device.session_id; - if (member && !sessionIdChanged) { - log.set("update", true); - member.updateCallInfo(device, log); - } else { - if (member && sessionIdChanged) { - log.set("removedSessionId", member.sessionId); - const disconnectLogItem = member.disconnect(false); - if (disconnectLogItem) { - log.refDetached(disconnectLogItem); - } - member.dispose(); - this._members.remove(memberKey); - member = undefined; - } - log.set("add", true); - member = new Member( - roomMember, - device, this._memberOptions, - log - ); - this._members.add(memberKey, member); - if (this.joinedData) { - this.connectToMember(member, this.joinedData, log); - } - } - // flush pending messages, either after having created the member, - // or updated the session id with updateCallInfo - this.flushPendingIncomingDeviceMessages(member, log); - }); - } - } - - const newDeviceIds = new Set(devices.map(call => call.device_id)); - // remove user as member of any calls not present anymore - for (const previousDeviceId of previousDeviceIds) { - if (!newDeviceIds.has(previousDeviceId)) { - this.removeMemberDevice(userId, previousDeviceId, log); - } - } - if (userId === this.options.ownUserId && !newDeviceIds.has(this.options.ownDeviceId)) { - this.removeOwnDevice(log); - } + }); + } + + /** @internal */ + updateMembership(userId: string, roomMember: RoomMember, callMembership: CallMembership, syncLog: ILogItem) { + this.errorBoundary.try(() => { + syncLog.wrap({l: "update call membership", t: CALL_LOG_TYPE, id: this.id, userId}, log => { + const now = this.options.clock.now(); + const devices = callMembership["m.devices"]; + const previousDeviceIds = this.getDeviceIdsForUserId(userId); + for (const device of devices) { + const deviceId = device.device_id; + const memberKey = getMemberKey(userId, deviceId); + if (userId === this.options.ownUserId && deviceId === this.options.ownDeviceId) { + log.wrap("update own membership", log => { + if (this.hasJoined) { + if (this.joinedData) { + this.joinedData.logItem.refDetached(log); + } + this._setupRenewMembershipTimeout(device, log); + } + if (this._state === GroupCallState.Joining) { + log.set("joined", true); + this._state = GroupCallState.Joined; + this.emitChange(); + } + }); + } else { + log.wrap({l: "update device membership", id: memberKey, sessionId: device.session_id}, log => { + if (isMemberExpired(device, now)) { + log.set("expired", true); + const member = this._members.get(memberKey); + if (member) { + member.dispose(); + this._members.remove(memberKey); + log.set("removed", true); + } + return; + } + let member = this._members.get(memberKey); + const sessionIdChanged = member && member.sessionId !== device.session_id; + if (member && !sessionIdChanged) { + log.set("update", true); + member.updateCallInfo(device, log); + } else { + if (member && sessionIdChanged) { + log.set("removedSessionId", member.sessionId); + const disconnectLogItem = member.disconnect(false); + if (disconnectLogItem) { + log.refDetached(disconnectLogItem); + } + member.dispose(); + this._members.remove(memberKey); + member = undefined; + } + log.set("add", true); + member = new Member( + roomMember, + device, this._memberOptions, + log + ); + this._members.add(memberKey, member); + if (this.joinedData) { + this.connectToMember(member, this.joinedData, log); + } + } + // flush pending messages, either after having created the member, + // or updated the session id with updateCallInfo + this.flushPendingIncomingDeviceMessages(member, log); + }); + } + } + + const newDeviceIds = new Set(devices.map(call => call.device_id)); + // remove user as member of any calls not present anymore + for (const previousDeviceId of previousDeviceIds) { + if (!newDeviceIds.has(previousDeviceId)) { + this.removeMemberDevice(userId, previousDeviceId, log); + } + } + if (userId === this.options.ownUserId && !newDeviceIds.has(this.options.ownDeviceId)) { + this.removeOwnDevice(log); + } + }); }); } /** @internal */ removeMembership(userId: string, syncLog: ILogItem) { - const deviceIds = this.getDeviceIdsForUserId(userId); - syncLog.wrap({ - l: "remove call member", - t: CALL_LOG_TYPE, - id: this.id, - userId - }, log => { - for (const deviceId of deviceIds) { - this.removeMemberDevice(userId, deviceId, log); - } - if (userId === this.options.ownUserId) { - this.removeOwnDevice(log); - } + this.errorBoundary.try(() => { + const deviceIds = this.getDeviceIdsForUserId(userId); + syncLog.wrap({ + l: "remove call member", + t: CALL_LOG_TYPE, + id: this.id, + userId + }, log => { + for (const deviceId of deviceIds) { + this.removeMemberDevice(userId, deviceId, log); + } + if (userId === this.options.ownUserId) { + this.removeOwnDevice(log); + } + }); }); } @@ -465,19 +494,21 @@ export class GroupCall extends EventEmitter<{change: never}> { } /** @internal */ - disconnect(log: ILogItem) { - if (this.hasJoined) { - for (const [,member] of this._members) { - const disconnectLogItem = member.disconnect(true); - if (disconnectLogItem) { - log.refDetached(disconnectLogItem); + disconnect(log: ILogItem): boolean { + return this.errorBoundary.try(() => { + if (this.hasJoined) { + for (const [,member] of this._members) { + const disconnectLogItem = member.disconnect(true); + if (disconnectLogItem) { + log.refDetached(disconnectLogItem); + } } + this._state = GroupCallState.Created; } - this._state = GroupCallState.Created; - } - this.joinedData?.dispose(); - this.joinedData = undefined; - this.emitChange(); + this.joinedData?.dispose(); + this.joinedData = undefined; + this.emitChange(); + }, false) || true; } /** @internal */ @@ -500,31 +531,33 @@ export class GroupCall extends EventEmitter<{change: never}> { /** @internal */ handleDeviceMessage(message: SignallingMessage, userId: string, deviceId: string, syncLog: ILogItem) { - // TODO: return if we are not membering to the call - const key = getMemberKey(userId, deviceId); - let member = this._members.get(key); - if (member && message.content.sender_session_id === member.sessionId) { - member.handleDeviceMessage(message, syncLog); - } else { - const item = syncLog.log({ - l: "call: buffering to_device message, member not found", - t: CALL_LOG_TYPE, - id: this.id, - userId, - deviceId, - sessionId: message.content.sender_session_id, - type: message.type - }); - syncLog.refDetached(item); - // we haven't received the m.call.member yet for this caller (or with this session id). - // buffer the device messages or create the member/call as it should arrive in a moment - let messages = this.bufferedDeviceMessages.get(key); - if (!messages) { - messages = new Set(); - this.bufferedDeviceMessages.set(key, messages); + this.errorBoundary.try(() => { + // TODO: return if we are not membering to the call + const key = getMemberKey(userId, deviceId); + let member = this._members.get(key); + if (member && message.content.sender_session_id === member.sessionId) { + member.handleDeviceMessage(message, syncLog); + } else { + const item = syncLog.log({ + l: "call: buffering to_device message, member not found", + t: CALL_LOG_TYPE, + id: this.id, + userId, + deviceId, + sessionId: message.content.sender_session_id, + type: message.type + }); + syncLog.refDetached(item); + // we haven't received the m.call.member yet for this caller (or with this session id). + // buffer the device messages or create the member/call as it should arrive in a moment + let messages = this.bufferedDeviceMessages.get(key); + if (!messages) { + messages = new Set(); + this.bufferedDeviceMessages.set(key, messages); + } + messages.add(message); } - messages.add(message); - } + }); } private async _createMemberPayload(includeOwn: boolean): Promise { From fef7af3b31fa4fb48f2d72a038493beac5f915cc Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Mon, 9 Jan 2023 14:04:13 +0100 Subject: [PATCH 05/32] report errors from ErrorBoundary on GroupCall and Member in UI UI is still very crude fwiw --- src/domain/session/room/CallViewModel.ts | 19 ++++++++++++++++++- .../session/room/timeline/tiles/CallTile.js | 11 ++++++++--- .../web/ui/css/themes/element/call.css | 16 ++++++++++++++++ src/platform/web/ui/session/room/CallView.ts | 8 +++++++- 4 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/domain/session/room/CallViewModel.ts b/src/domain/session/room/CallViewModel.ts index 37f30840..358b26f0 100644 --- a/src/domain/session/room/CallViewModel.ts +++ b/src/domain/session/room/CallViewModel.ts @@ -39,7 +39,11 @@ export class CallViewModel extends ViewModel { constructor(options: Options) { super(options); - const ownMemberViewModelMap = new ObservableValueMap("self", new EventObservableValue(this.call, "change")) + const callObservable = new EventObservableValue(this.call, "change"); + this.track(callObservable.subscribe(() => { + this.emitChange(); + })); + const ownMemberViewModelMap = new ObservableValueMap("self", callObservable) .mapValues((call, emitChange) => new OwnMemberViewModel(this.childOptions({call, emitChange})), () => {}); this.memberViewModels = this.call.members .filterValues(member => member.isConnected) @@ -79,6 +83,10 @@ export class CallViewModel extends ViewModel { return this.call.id; } + get error(): string | undefined { + return this.call.error?.message; + } + private get call(): GroupCall { return this.getOption("call"); } @@ -135,6 +143,10 @@ class OwnMemberViewModel extends ViewModel implements IStreamViewModel })); } + get error(): string | undefined { + return undefined; + } + get stream(): Stream | undefined { return this.call.localPreviewMedia?.userMedia; } @@ -195,6 +207,10 @@ export class CallMemberViewModel extends ViewModel implements ISt return this.member.remoteMedia?.userMedia; } + get error(): string | undefined { + return this.member.error?.message; + } + private get member(): Member { return this.getOption("member"); } @@ -242,4 +258,5 @@ export interface IStreamViewModel extends AvatarSource, ViewModel { get stream(): Stream | undefined; get isCameraMuted(): boolean; get isMicrophoneMuted(): boolean; + get error(): string | undefined; } diff --git a/src/domain/session/room/timeline/tiles/CallTile.js b/src/domain/session/room/timeline/tiles/CallTile.js index 0bc12698..a54af5d8 100644 --- a/src/domain/session/room/timeline/tiles/CallTile.js +++ b/src/domain/session/room/timeline/tiles/CallTile.js @@ -74,9 +74,14 @@ export class CallTile extends SimpleTile { async join() { if (this.canJoin) { - const stream = await this.platform.mediaDevices.getMediaTracks(false, true); - const localMedia = new LocalMedia().withUserMedia(stream); - await this._call.join(localMedia); + try { + const stream = await this.platform.mediaDevices.getMediaTracks(false, true); + const localMedia = new LocalMedia().withUserMedia(stream); + await this._call.join(localMedia); + } catch (err) { + this._error = err; + this.emitChange("error"); + } } } diff --git a/src/platform/web/ui/css/themes/element/call.css b/src/platform/web/ui/css/themes/element/call.css index 10388fb4..f4d674b4 100644 --- a/src/platform/web/ui/css/themes/element/call.css +++ b/src/platform/web/ui/css/themes/element/call.css @@ -24,6 +24,14 @@ limitations under the License. grid-row: 1; } +.CallView_error { + color: red; + font-weight: bold; + align-self: start; + justify-self: center; + margin: 16px; +} + .CallView_members { display: grid; gap: 12px; @@ -59,6 +67,14 @@ limitations under the License. justify-self: center; } +.StreamView_error { + color: red; + font-weight: bold; + align-self: start; + justify-self: center; + margin: 16px; +} + .StreamView_muteStatus { align-self: start; justify-self: end; diff --git a/src/platform/web/ui/session/room/CallView.ts b/src/platform/web/ui/session/room/CallView.ts index 619afc2e..87cac99f 100644 --- a/src/platform/web/ui/session/room/CallView.ts +++ b/src/platform/web/ui/session/room/CallView.ts @@ -43,7 +43,10 @@ export class CallView extends TemplateView { "CallView_unmutedCamera": vm => !vm.isCameraMuted, }, onClick: disableTargetCallback(() => vm.toggleCamera())}), t.button({className: "CallView_hangup", onClick: disableTargetCallback(() => vm.hangup())}), - ]) + ]), + t.if(vm => !!vm.error, t => { + return t.div({className: "CallView_error"}, vm => vm.error); + }) ]); } @@ -112,6 +115,9 @@ class StreamView extends TemplateView { microphoneMuted: vm => vm.isMicrophoneMuted && !vm.isCameraMuted, cameraMuted: vm => vm.isCameraMuted, } + }), + t.if(vm => !!vm.error, t => { + return t.div({className: "StreamView_error"}, vm => vm.error); }) ]); } From bd3499056ae358d1de1f172dc33e2d3f939a827d Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 10 Jan 2023 12:05:30 +0100 Subject: [PATCH 06/32] provider higher-level rageshake fn for opened session Co-authored-by: R Midhun Suresh --- src/domain/rageshake.ts | 33 +++++++++++++++++-- .../session/settings/SettingsViewModel.js | 30 ++++------------- 2 files changed, 38 insertions(+), 25 deletions(-) diff --git a/src/domain/rageshake.ts b/src/domain/rageshake.ts index cb06e638..08e82d8c 100644 --- a/src/domain/rageshake.ts +++ b/src/domain/rageshake.ts @@ -16,11 +16,15 @@ limitations under the License. import type {BlobHandle} from "../platform/web/dom/BlobHandle"; import type {RequestFunction} from "../platform/types/types"; +import type {Platform} from "../platform/web/Platform"; +import type {ILogger} from "../logging/types"; +import type { IDBLogPersister } from "../logging/IDBLogPersister"; +import type { Session } from "../matrix/Session"; // see https://github.com/matrix-org/rageshake#readme type RageshakeData = { // A textual description of the problem. Included in the details.log.gz file. - text: string | undefined; + text?: string; // Application user-agent. Included in the details.log.gz file. userAgent: string; // Identifier for the application (eg 'riot-web'). Should correspond to a mapping configured in the configuration file for github issue reporting to work. @@ -28,7 +32,7 @@ type RageshakeData = { // Application version. Included in the details.log.gz file. version: string; // Label to attach to the github issue, and include in the details file. - label: string | undefined; + label?: string; }; export async function submitLogsToRageshakeServer(data: RageshakeData, logsBlob: BlobHandle, submitUrl: string, request: RequestFunction): Promise { @@ -63,3 +67,28 @@ export async function submitLogsToRageshakeServer(data: RageshakeData, logsBlob: // we don't bother with reading report_url from the body as the rageshake server doesn't always return it // and would have to have CORS setup properly for us to be able to read it. } + +/** @throws {Error} */ +export async function submitLogsFromSessionToDefaultServer(session: Session, platform: Platform): Promise { + const {bugReportEndpointUrl} = platform.config; + if (!bugReportEndpointUrl) { + throw new Error("no server configured to submit logs"); + } + const logReporters = (platform.logger as ILogger).reporters; + const exportReporter = logReporters.find(r => !!r["export"]) as IDBLogPersister | undefined; + if (!exportReporter) { + throw new Error("No logger that can export configured"); + } + const logExport = await exportReporter.export(); + await submitLogsToRageshakeServer( + { + app: "hydrogen", + userAgent: platform.description, + version: platform.version, + text: `Submit logs from settings for user ${session.userId} on device ${session.deviceId}`, + }, + logExport.asBlob(), + bugReportEndpointUrl, + platform.request + ); +} \ No newline at end of file diff --git a/src/domain/session/settings/SettingsViewModel.js b/src/domain/session/settings/SettingsViewModel.js index 0d61bcac..d60a6327 100644 --- a/src/domain/session/settings/SettingsViewModel.js +++ b/src/domain/session/settings/SettingsViewModel.js @@ -16,7 +16,7 @@ limitations under the License. import {ViewModel} from "../../ViewModel"; import {KeyBackupViewModel} from "./KeyBackupViewModel.js"; -import {submitLogsToRageshakeServer} from "../../../domain/rageshake"; +import {submitLogsFromSessionToDefaultServer} from "../../../domain/rageshake"; class PushNotificationStatus { constructor() { @@ -175,29 +175,13 @@ export class SettingsViewModel extends ViewModel { } async sendLogsToServer() { - const {bugReportEndpointUrl} = this.platform.config; - if (bugReportEndpointUrl) { - this._logsFeedbackMessage = this.i18n`Sending logs…`; + this._logsFeedbackMessage = this.i18n`Sending logs…`; + try { + await submitLogsFromSessionToDefaultServer(this._session, this.platform); + this._logsFeedbackMessage = this.i18n`Logs sent succesfully!`; + } catch (err) { + this._logsFeedbackMessage = err.message; this.emitChange(); - try { - const logExport = await this.logger.export(); - await submitLogsToRageshakeServer( - { - app: "hydrogen", - userAgent: this.platform.description, - version: DEFINE_VERSION, - text: `Submit logs from settings for user ${this._session.userId} on device ${this._session.deviceId}`, - }, - logExport.asBlob(), - bugReportEndpointUrl, - this.platform.request - ); - this._logsFeedbackMessage = this.i18n`Logs sent succesfully!`; - this.emitChange(); - } catch (err) { - this._logsFeedbackMessage = err.message; - this.emitChange(); - } } } From b8bc6edbc0240731b883437a8a062f1f0d36c645 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 10 Jan 2023 12:06:49 +0100 Subject: [PATCH 07/32] add ErrorView(Model) to easily report errors and submit logs from UI --- src/domain/ErrorViewModel.ts | 44 +++++++++++++++ .../web/ui/css/themes/element/error.css | 50 +++++++++++++++++ .../web/ui/css/themes/element/theme.css | 1 + src/platform/web/ui/general/ErrorView.ts | 56 +++++++++++++++++++ 4 files changed, 151 insertions(+) create mode 100644 src/domain/ErrorViewModel.ts create mode 100644 src/platform/web/ui/css/themes/element/error.css create mode 100644 src/platform/web/ui/general/ErrorView.ts diff --git a/src/domain/ErrorViewModel.ts b/src/domain/ErrorViewModel.ts new file mode 100644 index 00000000..9045af80 --- /dev/null +++ b/src/domain/ErrorViewModel.ts @@ -0,0 +1,44 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { ViewModel, Options as BaseOptions } from "./ViewModel"; +import {submitLogsFromSessionToDefaultServer} from "./rageshake"; +import type { Session } from "../matrix/Session"; + +type Options = { + error: Error + session: Session, + onClose: () => void +} & BaseOptions; + +export class ErrorViewModel extends ViewModel { + get message(): string { + return this.getOption("error")?.message; + } + + close() { + this.getOption("onClose")(); + } + + async submitLogs(): Promise { + try { + await submitLogsFromSessionToDefaultServer(this.getOption("session"), this.platform); + return true; + } catch (err) { + return false; + } + } +} \ No newline at end of file diff --git a/src/platform/web/ui/css/themes/element/error.css b/src/platform/web/ui/css/themes/element/error.css new file mode 100644 index 00000000..f4e4fb6f --- /dev/null +++ b/src/platform/web/ui/css/themes/element/error.css @@ -0,0 +1,50 @@ +.ErrorView_block { + background: var(--error-color); + color: var(--fixed-white); + margin: 16px; +} + +.ErrorView_inline { + color: var(--error-color); + margin: 4px; +} + +.ErrorView { + font-weight: bold; + margin: 16px; + border-radius: 8px; + padding: 12px; + display: flex; + gap: 4px; +} + +.ErrorView_message { + flex-basis: 0; + flex-grow: 1; + margin: 0px; + word-break: break-all; + word-break: break-word; +} + +.ErrorView_submit { + align-self: end; +} + +.ErrorView_close { + align-self: start; + width: 16px; + height: 16px; + border: none; + background: none; + background-repeat: no-repeat; + background-size: contain; + cursor: pointer; +} + +.ErrorView_block .ErrorView_close { + background-image: url('icons/clear.svg?primary=fixed-white'); +} + +.ErrorView_inline .ErrorView_close { + background-image: url('icons/clear.svg?primary=text-color'); +} \ No newline at end of file diff --git a/src/platform/web/ui/css/themes/element/theme.css b/src/platform/web/ui/css/themes/element/theme.css index 6403bb60..84a27f13 100644 --- a/src/platform/web/ui/css/themes/element/theme.css +++ b/src/platform/web/ui/css/themes/element/theme.css @@ -19,6 +19,7 @@ limitations under the License. @import url('inter.css'); @import url('timeline.css'); @import url('call.css'); +@import url('error.css'); :root { font-size: 10px; diff --git a/src/platform/web/ui/general/ErrorView.ts b/src/platform/web/ui/general/ErrorView.ts new file mode 100644 index 00000000..f44361c3 --- /dev/null +++ b/src/platform/web/ui/general/ErrorView.ts @@ -0,0 +1,56 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import {TemplateView, Builder} from "./TemplateView"; +import { disableTargetCallback } from "./utils"; + +import type { ViewNode } from "./types"; +import type {ErrorViewModel} from "../../../../domain/ErrorViewModel"; + + +export class ErrorView extends TemplateView { + constructor(vm: ErrorViewModel, private readonly options: {inline: boolean} = {inline: false}) { + super(vm); + } + override render(t: Builder, vm: ErrorViewModel): ViewNode { + const submitLogsButton = t.button({ + className: "ErrorView_submit", + onClick: disableTargetCallback(async () => { + if (await vm.submitLogs()) { + alert("Logs submitted!"); + } else { + alert("Could not submit logs"); + } + }) + }, "Submit logs"); + const closeButton = t.button({ + className: "ErrorView_close", + onClick: () => vm.close(), + title: "Dismiss error" + }); + return t.div({ + className: { + "ErrorView": true, + "ErrorView_inline": this.options.inline, + "ErrorView_block": !this.options.inline + }}, [ + t.p({className: "ErrorView_message"}, vm.message), + submitLogsButton, + closeButton + ]); + } +} + From f15e849f5496bca0b571c9f47d5f069882e38e82 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 10 Jan 2023 12:08:10 +0100 Subject: [PATCH 08/32] user error view model in room, also when starting call --- src/domain/session/room/RoomViewModel.js | 68 +++++++++++--------- src/platform/web/ui/session/room/RoomView.js | 5 +- 2 files changed, 40 insertions(+), 33 deletions(-) diff --git a/src/domain/session/room/RoomViewModel.js b/src/domain/session/room/RoomViewModel.js index 077a996e..d744e5d5 100644 --- a/src/domain/session/room/RoomViewModel.js +++ b/src/domain/session/room/RoomViewModel.js @@ -23,6 +23,7 @@ import {avatarInitials, getIdentifierColorNumber, getAvatarHttpUrl} from "../../ import {ViewModel} from "../../ViewModel"; import {imageToInfo} from "../common.js"; import {LocalMedia} from "../../../matrix/calls/LocalMedia"; +import { ErrorViewModel } from "../../ErrorViewModel"; // TODO: remove fallback so default isn't included in bundle for SDK users that have their custom tileClassForEntry // this is a breaking SDK change though to make this option mandatory import {tileClassForEntry as defaultTileClassForEntry} from "./timeline/tiles/index"; @@ -36,8 +37,7 @@ export class RoomViewModel extends ViewModel { this._tileClassForEntry = tileClassForEntry ?? defaultTileClassForEntry; this._tileOptions = undefined; this._onRoomChange = this._onRoomChange.bind(this); - this._timelineError = null; - this._sendError = null; + this._errorViewModel = null; this._composerVM = null; if (room.isArchived) { this._composerVM = new ArchivedViewModel(this.childOptions({archivedRoom: room})); @@ -73,6 +73,14 @@ export class RoomViewModel extends ViewModel { } } + _reportError(error) { + this._errorViewModel = new ErrorViewModel(this.childOptions({error, onClose: () => { + this._errorViewModel = null; + this.emitChange("errorViewModel"); + }})); + this.emitChange("errorViewModel"); + } + async load() { this._room.on("change", this._onRoomChange); try { @@ -88,10 +96,8 @@ export class RoomViewModel extends ViewModel { timeline, }))); this.emitChange("timelineViewModel"); - } catch (err) { - console.error(`room.openTimeline(): ${err.message}:\n${err.stack}`); - this._timelineError = err; - this.emitChange("error"); + } catch (error) { + this._reportError(error); } this._clearUnreadAfterDelay(); } @@ -143,14 +149,8 @@ export class RoomViewModel extends ViewModel { get timelineViewModel() { return this._timelineVM; } get isEncrypted() { return this._room.isEncrypted; } - get error() { - if (this._timelineError) { - return `Something went wrong loading the timeline: ${this._timelineError.message}`; - } - if (this._sendError) { - return `Something went wrong sending your message: ${this._sendError.message}`; - } - return ""; + get errorViewModel() { + return this._errorViewModel; } get avatarLetter() { @@ -215,11 +215,8 @@ export class RoomViewModel extends ViewModel { } else { await this._room.sendEvent("m.room.message", {msgtype, body: message}); } - } catch (err) { - console.error(`room.sendMessage(): ${err.message}:\n${err.stack}`); - this._sendError = err; - this._timelineError = null; - this.emitChange("error"); + } catch (error) { + this._reportError(error); return false; } return true; @@ -289,10 +286,8 @@ export class RoomViewModel extends ViewModel { attachments["info.thumbnail_url"] = this._room.createAttachment(thumbnail.blob, file.name); await this._room.sendEvent("m.room.message", content, attachments); - } catch (err) { - this._sendError = err; - this.emitChange("error"); - console.error(err.stack); + } catch (error) { + this._reportError(error); } } @@ -331,10 +326,8 @@ export class RoomViewModel extends ViewModel { this._room.createAttachment(thumbnail.blob, file.name); } await this._room.sendEvent("m.room.message", content, attachments); - } catch (err) { - this._sendError = err; - this.emitChange("error"); - console.error(err.stack); + } catch (error) { + this._reportError(error); } } @@ -364,15 +357,28 @@ export class RoomViewModel extends ViewModel { } async startCall() { + let localMedia; try { - const session = this.getOption("session"); const stream = await this.platform.mediaDevices.getMediaTracks(false, true); - const localMedia = new LocalMedia().withUserMedia(stream); + localMedia = new LocalMedia().withUserMedia(stream); + } catch (err) { + this._reportError(new Error(`Could not get local audio and/or video stream: ${err.message}`)); + return; + } + const session = this.getOption("session"); + let call; + try { // this will set the callViewModel above as a call will be added to callHandler.calls - const call = await session.callHandler.createCall(this._room.id, "m.video", "A call " + Math.round(this.platform.random() * 100)); + call = await session.callHandler.createCall(this._room.id, "m.video", "A call " + Math.round(this.platform.random() * 100)); + } catch (err) { + this._reportError(new Error(`Could not create call: ${err.message}`)); + return; + } + try { await call.join(localMedia); } catch (err) { - console.error(err.stack); + this._reportError(new Error(`Could not join call: ${err.message}`)); + return; } } } diff --git a/src/platform/web/ui/session/room/RoomView.js b/src/platform/web/ui/session/room/RoomView.js index 4f9e40d9..9478f5e6 100644 --- a/src/platform/web/ui/session/room/RoomView.js +++ b/src/platform/web/ui/session/room/RoomView.js @@ -15,7 +15,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import {TemplateView} from "../../general/TemplateView"; +import {TemplateView} from "../../general/TemplateView"; import {Popup} from "../../general/Popup.js"; import {Menu} from "../../general/Menu.js"; import {TimelineView} from "./TimelineView"; @@ -24,6 +24,7 @@ import {MessageComposer} from "./MessageComposer.js"; import {RoomArchivedView} from "./RoomArchivedView.js"; import {AvatarView} from "../../AvatarView.js"; import {CallView} from "./CallView"; +import { ErrorView } from "../../general/ErrorView"; export class RoomView extends TemplateView { constructor(vm, viewClassForTile) { @@ -53,7 +54,7 @@ export class RoomView extends TemplateView { }) ]), t.div({className: "RoomView_body"}, [ - t.div({className: "RoomView_error"}, vm => vm.error), + t.if(vm => vm.errorViewModel, t => t.div({className: "RoomView_error"}, t.view(new ErrorView(vm.errorViewModel)))), t.mapView(vm => vm.callViewModel, callViewModel => callViewModel ? new CallView(callViewModel) : null), t.mapView(vm => vm.timelineViewModel, timelineViewModel => { return timelineViewModel ? From 42ee2d294b1f9f4f3afd3d6ccb737d70ccf95bb6 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 10 Jan 2023 12:09:10 +0100 Subject: [PATCH 09/32] use error view model from call tile --- .../session/room/timeline/tiles/CallTile.js | 26 +++++++++++++++---- .../ui/session/room/timeline/CallTileView.ts | 21 ++++++++++----- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/domain/session/room/timeline/tiles/CallTile.js b/src/domain/session/room/timeline/tiles/CallTile.js index a54af5d8..a3008cb9 100644 --- a/src/domain/session/room/timeline/tiles/CallTile.js +++ b/src/domain/session/room/timeline/tiles/CallTile.js @@ -16,7 +16,7 @@ limitations under the License. import {SimpleTile} from "./SimpleTile.js"; import {LocalMedia} from "../../../../../matrix/calls/LocalMedia"; - +import {ErrorViewModel} from "../../../../ErrorViewModel" // TODO: timeline entries for state events with the same state key and type // should also update previous entries in the timeline, so we can update the name of the call, whether it is terminated, etc ... @@ -28,6 +28,7 @@ export class CallTile extends SimpleTile { const calls = this.getOption("session").callHandler.calls; this._call = calls.get(this._entry.stateKey); this._callSubscription = undefined; + this._errorViewModel = undefined; if (this._call) { this._callSubscription = this._call.disposableOn("change", () => { // unsubscribe when terminated @@ -60,6 +61,10 @@ export class CallTile extends SimpleTile { return this._call && this._call.hasJoined; } + get errorViewModel() { + return this._errorViewModel; + } + get label() { if (this._call) { if (this._call.hasJoined) { @@ -78,16 +83,19 @@ export class CallTile extends SimpleTile { const stream = await this.platform.mediaDevices.getMediaTracks(false, true); const localMedia = new LocalMedia().withUserMedia(stream); await this._call.join(localMedia); - } catch (err) { - this._error = err; - this.emitChange("error"); + } catch (error) { + this._reportError(error); } } } async leave() { if (this.canLeave) { - this._call.leave(); + try { + this._call.leave(); + } catch (err) { + this._reportError(err); + } } } @@ -96,4 +104,12 @@ export class CallTile extends SimpleTile { this._callSubscription = this._callSubscription(); } } + + _reportError(error) { + this._errorViewModel = new ErrorViewModel(this.childOptions({error, onClose: () => { + this._errorViewModel = undefined; + this.emitChange("errorViewModel"); + }})); + this.emitChange("errorViewModel"); + } } diff --git a/src/platform/web/ui/session/room/timeline/CallTileView.ts b/src/platform/web/ui/session/room/timeline/CallTileView.ts index e0ca00bd..5bede510 100644 --- a/src/platform/web/ui/session/room/timeline/CallTileView.ts +++ b/src/platform/web/ui/session/room/timeline/CallTileView.ts @@ -14,17 +14,24 @@ See the License for the specific language governing permissions and limitations under the License. */ -import {TemplateView} from "../../../general/TemplateView"; +import {Builder, TemplateView} from "../../../general/TemplateView"; import type {CallTile} from "../../../../../../domain/session/room/timeline/tiles/CallTile"; +import {ErrorView} from "../../../general/ErrorView"; export class CallTileView extends TemplateView { - render(t, vm) { + render(t: Builder, vm: CallTile) { return t.li( - {className: "AnnouncementView"}, - t.div([ - vm => vm.label, - t.button({className: "CallTileView_join", hidden: vm => !vm.canJoin}, "Join"), - t.button({className: "CallTileView_leave", hidden: vm => !vm.canLeave}, "Leave") + {className: "CallTileView AnnouncementView"}, + t.div( + [ + t.if(vm => vm.errorViewModel, t => { + return t.div({className: "CallTileView_error"}, t.view(new ErrorView(vm.errorViewModel, {inline: true}))); + }), + t.div([ + vm => vm.label, + t.button({className: "CallTileView_join", hidden: vm => !vm.canJoin}, "Join"), + t.button({className: "CallTileView_leave", hidden: vm => !vm.canLeave}, "Leave") + ]) ]) ); } From 64d6db556a0e3a6a0c5fed3dc8a27345962e928f Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 12 Jan 2023 14:36:39 +0100 Subject: [PATCH 10/32] fix updates from call and member classes in VM this fixes this.emitChange sending the update over the collection in the call member VM, which is how updates are subscribed to by the UI. It also adds a callback to the VM for when the member sends an update, so we can check later on if the error on the member has been set. --- src/domain/session/room/CallViewModel.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/domain/session/room/CallViewModel.ts b/src/domain/session/room/CallViewModel.ts index 358b26f0..1080015f 100644 --- a/src/domain/session/room/CallViewModel.ts +++ b/src/domain/session/room/CallViewModel.ts @@ -40,14 +40,19 @@ export class CallViewModel extends ViewModel { constructor(options: Options) { super(options); const callObservable = new EventObservableValue(this.call, "change"); - this.track(callObservable.subscribe(() => { - this.emitChange(); - })); + this.track(callObservable.subscribe(() => this.onUpdate())); const ownMemberViewModelMap = new ObservableValueMap("self", callObservable) .mapValues((call, emitChange) => new OwnMemberViewModel(this.childOptions({call, emitChange})), () => {}); this.memberViewModels = this.call.members .filterValues(member => member.isConnected) - .mapValues(member => new CallMemberViewModel(this.childOptions({member, mediaRepository: this.getOption("room").mediaRepository}))) + .mapValues( + (member, emitChange) => new CallMemberViewModel(this.childOptions({ + member, + emitChange, + mediaRepository: this.getOption("room").mediaRepository + })), + (vm: CallMemberViewModel) => vm.onUpdate(), + ) .join(ownMemberViewModelMap) .sortValues((a, b) => a.compare(b)); this.track(this.memberViewModels.subscribe({ @@ -91,6 +96,9 @@ export class CallViewModel extends ViewModel { return this.getOption("call"); } + private onUpdate() { + } + async hangup() { if (this.call.hasJoined) { await this.call.leave(); @@ -241,6 +249,8 @@ export class CallMemberViewModel extends ViewModel implements ISt return this.member.member.name; } + onUpdate() { + } compare(other: OwnMemberViewModel | CallMemberViewModel): number { if (other instanceof OwnMemberViewModel) { return -other.compare(this); From 4070d422cdd0883af3c4709cab3afc781c13c14b Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 12 Jan 2023 14:37:28 +0100 Subject: [PATCH 11/32] use error view (model) in call view (model) --- src/domain/ErrorViewModel.ts | 4 ++ src/domain/session/room/CallViewModel.ts | 68 ++++++++++++++++---- src/platform/web/ui/session/room/CallView.ts | 9 +-- 3 files changed, 65 insertions(+), 16 deletions(-) diff --git a/src/domain/ErrorViewModel.ts b/src/domain/ErrorViewModel.ts index 9045af80..51a34565 100644 --- a/src/domain/ErrorViewModel.ts +++ b/src/domain/ErrorViewModel.ts @@ -29,6 +29,10 @@ export class ErrorViewModel extends ViewModel { return this.getOption("error")?.message; } + get error(): Error { + return this.getOption("error"); + } + close() { this.getOption("onClose")(); } diff --git a/src/domain/session/room/CallViewModel.ts b/src/domain/session/room/CallViewModel.ts index 1080015f..e87af12a 100644 --- a/src/domain/session/room/CallViewModel.ts +++ b/src/domain/session/room/CallViewModel.ts @@ -20,6 +20,7 @@ import {getStreamVideoTrack, getStreamAudioTrack} from "../../../matrix/calls/co import {avatarInitials, getIdentifierColorNumber, getAvatarHttpUrl} from "../../avatar"; import {EventObservableValue} from "../../../observable/value/EventObservableValue"; import {ObservableValueMap} from "../../../observable/map/ObservableValueMap"; +import { ErrorViewModel } from "../../ErrorViewModel"; import type {Room} from "../../../matrix/room/Room"; import type {GroupCall} from "../../../matrix/calls/group/GroupCall"; import type {Member} from "../../../matrix/calls/group/Member"; @@ -28,14 +29,17 @@ import type {BaseObservableList} from "../../../observable/list/BaseObservableLi import type {BaseObservableValue} from "../../../observable/value/BaseObservableValue"; import type {Stream} from "../../../platform/types/MediaDevices"; import type {MediaRepository} from "../../../matrix/net/MediaRepository"; +import type { Session } from "../../../matrix/Session"; type Options = BaseOptions & { call: GroupCall, room: Room, + session: Session }; export class CallViewModel extends ViewModel { public readonly memberViewModels: BaseObservableList; + private _errorViewModel?: ErrorViewModel; constructor(options: Options) { super(options); @@ -88,8 +92,8 @@ export class CallViewModel extends ViewModel { return this.call.id; } - get error(): string | undefined { - return this.call.error?.message; + get errorViewModel(): ErrorViewModel | undefined { + return this._errorViewModel; } private get call(): GroupCall { @@ -97,11 +101,18 @@ export class CallViewModel extends ViewModel { } private onUpdate() { + if (this.call.error) { + this._reportError(this.call.error); + } } async hangup() { - if (this.call.hasJoined) { - await this.call.leave(); + try { + if (this.call.hasJoined) { + await this.call.leave(); + } + } catch (err) { + this._reportError(err); } } @@ -125,7 +136,6 @@ export class CallViewModel extends ViewModel { // unmute but no track? if (muteSettings.microphone && !getStreamAudioTrack(localMedia.userMedia)) { const stream = await this.platform.mediaDevices.getMediaTracks(true, !muteSettings.camera); - console.log("got tracks", Array.from(stream.getTracks()).map((t: MediaStreamTrack) => { return {kind: t.kind, id: t.id};})) await this.call.setMedia(localMedia.withUserMedia(stream)); } else { await this.call.setMuted(muteSettings.toggleMicrophone()); @@ -133,6 +143,21 @@ export class CallViewModel extends ViewModel { this.emitChange(); } } + + private _reportError(error: Error) { + if (this._errorViewModel?.error === error) { + return; + } + this.disposeTracked(this._errorViewModel); + this._errorViewModel = new ErrorViewModel(this.childOptions({ + error, + onClose: () => { + this._errorViewModel = this.disposeTracked(this._errorViewModel); + this.emitChange("errorViewModel"); + } + })); + this.emitChange("errorViewModel"); + } } class OwnMemberViewModel extends ViewModel implements IStreamViewModel { @@ -151,7 +176,7 @@ class OwnMemberViewModel extends ViewModel implements IStreamViewModel })); } - get error(): string | undefined { + get errorViewModel(): ErrorViewModel | undefined { return undefined; } @@ -207,22 +232,25 @@ class OwnMemberViewModel extends ViewModel implements IStreamViewModel type MemberOptions = BaseOptions & { member: Member, - mediaRepository: MediaRepository + mediaRepository: MediaRepository, + session: Session }; export class CallMemberViewModel extends ViewModel implements IStreamViewModel { + private _errorViewModel?: ErrorViewModel; + get stream(): Stream | undefined { return this.member.remoteMedia?.userMedia; } - get error(): string | undefined { - return this.member.error?.message; - } - private get member(): Member { return this.getOption("member"); } + get errorViewModel(): ErrorViewModel | undefined { + return this._errorViewModel; + } + get isCameraMuted(): boolean { return this.member.remoteMuteSettings?.camera ?? true; } @@ -250,7 +278,23 @@ export class CallMemberViewModel extends ViewModel implements ISt } onUpdate() { + this.mapMemberSyncErrorIfNeeded(); } + + private mapMemberSyncErrorIfNeeded() { + if (this.member.error && (!this._errorViewModel || this._errorViewModel.error !== this.member.error)) { + this.disposeTracked(this._errorViewModel); + this._errorViewModel = this.track(new ErrorViewModel(this.childOptions({ + error: this.member.error, + onClose: () => { + this._errorViewModel = this.disposeTracked(this._errorViewModel); + this.emitChange("errorViewModel"); + }, + }))); + this.emitChange("errorViewModel"); + } + } + compare(other: OwnMemberViewModel | CallMemberViewModel): number { if (other instanceof OwnMemberViewModel) { return -other.compare(this); @@ -268,5 +312,5 @@ export interface IStreamViewModel extends AvatarSource, ViewModel { get stream(): Stream | undefined; get isCameraMuted(): boolean; get isMicrophoneMuted(): boolean; - get error(): string | undefined; + get errorViewModel(): ErrorViewModel | undefined; } diff --git a/src/platform/web/ui/session/room/CallView.ts b/src/platform/web/ui/session/room/CallView.ts index 87cac99f..eacb3144 100644 --- a/src/platform/web/ui/session/room/CallView.ts +++ b/src/platform/web/ui/session/room/CallView.ts @@ -20,6 +20,7 @@ import {ListView} from "../../general/ListView"; import {classNames} from "../../general/html"; import {Stream} from "../../../../types/MediaDevices"; import type {CallViewModel, CallMemberViewModel, IStreamViewModel} from "../../../../../domain/session/room/CallViewModel"; +import { ErrorView } from "../../general/ErrorView"; export class CallView extends TemplateView { private resizeObserver?: ResizeObserver; @@ -44,8 +45,8 @@ export class CallView extends TemplateView { }, onClick: disableTargetCallback(() => vm.toggleCamera())}), t.button({className: "CallView_hangup", onClick: disableTargetCallback(() => vm.hangup())}), ]), - t.if(vm => !!vm.error, t => { - return t.div({className: "CallView_error"}, vm => vm.error); + t.if(vm => !!vm.errorViewModel, t => { + return t.div({className: "CallView_error"}, t.view(new ErrorView(vm.errorViewModel!))); }) ]); } @@ -116,8 +117,8 @@ class StreamView extends TemplateView { cameraMuted: vm => vm.isCameraMuted, } }), - t.if(vm => !!vm.error, t => { - return t.div({className: "StreamView_error"}, vm => vm.error); + t.if(vm => !!vm.errorViewModel, t => { + return t.div({className: "StreamView_error"}, t.view(new ErrorView(vm.errorViewModel!))); }) ]); } From 7b32a2729e09c31778e9fb9f3c82f09586ddd499 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Fri, 13 Jan 2023 10:39:44 +0100 Subject: [PATCH 12/32] don't allow other click handlers to run in parent elements --- src/platform/web/ui/general/ErrorView.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/platform/web/ui/general/ErrorView.ts b/src/platform/web/ui/general/ErrorView.ts index f44361c3..eb1e7838 100644 --- a/src/platform/web/ui/general/ErrorView.ts +++ b/src/platform/web/ui/general/ErrorView.ts @@ -28,7 +28,8 @@ export class ErrorView extends TemplateView { override render(t: Builder, vm: ErrorViewModel): ViewNode { const submitLogsButton = t.button({ className: "ErrorView_submit", - onClick: disableTargetCallback(async () => { + onClick: disableTargetCallback(async evt => { + evt.stopPropagation(); if (await vm.submitLogs()) { alert("Logs submitted!"); } else { @@ -38,7 +39,10 @@ export class ErrorView extends TemplateView { }, "Submit logs"); const closeButton = t.button({ className: "ErrorView_close", - onClick: () => vm.close(), + onClick: evt => { + evt.stopPropagation(); + vm.close(); + }, title: "Dismiss error" }); return t.div({ From 75839007ea41b7a398a2adb95651629d5b8648ff Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Fri, 13 Jan 2023 10:40:04 +0100 Subject: [PATCH 13/32] make buttons clickable in the first place --- src/platform/web/ui/css/themes/element/call.css | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/platform/web/ui/css/themes/element/call.css b/src/platform/web/ui/css/themes/element/call.css index f4d674b4..c0f1550e 100644 --- a/src/platform/web/ui/css/themes/element/call.css +++ b/src/platform/web/ui/css/themes/element/call.css @@ -68,11 +68,12 @@ limitations under the License. } .StreamView_error { - color: red; - font-weight: bold; align-self: start; justify-self: center; - margin: 16px; + /** Chrome (v100) requires this to make the buttons clickable + * where they overlap with the video element, even though + * the buttons come later in the DOM. */ + z-index: 1; } .StreamView_muteStatus { From 2bba9f8675bfefc0122db7f0b1e8d7dc41f7c0ec Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Fri, 13 Jan 2023 10:40:15 +0100 Subject: [PATCH 14/32] some layout improvements --- src/platform/web/ui/css/themes/element/error.css | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/platform/web/ui/css/themes/element/error.css b/src/platform/web/ui/css/themes/element/error.css index f4e4fb6f..ce252056 100644 --- a/src/platform/web/ui/css/themes/element/error.css +++ b/src/platform/web/ui/css/themes/element/error.css @@ -15,7 +15,7 @@ border-radius: 8px; padding: 12px; display: flex; - gap: 4px; + gap: 8px; } .ErrorView_message { @@ -24,6 +24,7 @@ margin: 0px; word-break: break-all; word-break: break-word; + align-self: center; } .ErrorView_submit { From d3b5a7066383a8d8d0e3addc6ac1e6f232bdd8ab Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Fri, 13 Jan 2023 12:42:52 +0100 Subject: [PATCH 15/32] join errors thrown from matrix layer with sync errors caught by error boundary. this adds a new base view model that facilitates reporting errors with the ErrorViewModel --- src/domain/ErrorReportViewModel.ts | 62 +++++++++++ src/domain/session/room/CallViewModel.ts | 101 ++++++------------ src/domain/session/room/RoomViewModel.js | 27 ++--- .../session/room/timeline/tiles/CallTile.js | 31 ++---- .../session/room/timeline/tiles/SimpleTile.js | 4 +- 5 files changed, 114 insertions(+), 111 deletions(-) create mode 100644 src/domain/ErrorReportViewModel.ts diff --git a/src/domain/ErrorReportViewModel.ts b/src/domain/ErrorReportViewModel.ts new file mode 100644 index 00000000..eb6ed0aa --- /dev/null +++ b/src/domain/ErrorReportViewModel.ts @@ -0,0 +1,62 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { ViewModel } from "./ViewModel"; +import type { Options as BaseOptions } from "./ViewModel"; +import type { Session } from "../matrix/Session"; +import { ErrorViewModel } from "./ErrorViewModel"; +import type { LogCallback, LabelOrValues } from "../logging/types"; + +export type Options = BaseOptions & { + session: Session +}; + +/** Base class for view models that need to report errors to the UI. */ +export class ErrorReportViewModel extends ViewModel { + private _errorViewModel?: ErrorViewModel; + + get errorViewModel(): ErrorViewModel | undefined { + return this._errorViewModel; + } + + protected reportError(error: Error) { + if (this._errorViewModel?.error === error) { + return; + } + this.disposeTracked(this._errorViewModel); + this._errorViewModel = this.track(new ErrorViewModel(this.childOptions({ + error, + onClose: () => { + this._errorViewModel = this.disposeTracked(this._errorViewModel); + this.emitChange("errorViewModel"); + } + }))); + this.emitChange("errorViewModel"); + } + + protected logAndCatch(labelOrValues: LabelOrValues, callback: LogCallback, errorValue: T = undefined as unknown as T): T { + try { + const result = this.logger.run(labelOrValues, callback); + if (result instanceof Promise) { + result.catch(err => this.reportError(err)); + } + return result; + } catch (err) { + this.reportError(err); + return errorValue; + } + } +} diff --git a/src/domain/session/room/CallViewModel.ts b/src/domain/session/room/CallViewModel.ts index e87af12a..ee0ad502 100644 --- a/src/domain/session/room/CallViewModel.ts +++ b/src/domain/session/room/CallViewModel.ts @@ -15,7 +15,8 @@ limitations under the License. */ import {AvatarSource} from "../../AvatarSource"; -import {ViewModel, Options as BaseOptions} from "../../ViewModel"; +import type {ViewModel} from "../../ViewModel"; +import {ErrorReportViewModel, Options as BaseOptions} from "../../ErrorReportViewModel"; import {getStreamVideoTrack, getStreamAudioTrack} from "../../../matrix/calls/common"; import {avatarInitials, getIdentifierColorNumber, getAvatarHttpUrl} from "../../avatar"; import {EventObservableValue} from "../../../observable/value/EventObservableValue"; @@ -34,12 +35,10 @@ import type { Session } from "../../../matrix/Session"; type Options = BaseOptions & { call: GroupCall, room: Room, - session: Session }; -export class CallViewModel extends ViewModel { +export class CallViewModel extends ErrorReportViewModel { public readonly memberViewModels: BaseObservableList; - private _errorViewModel?: ErrorViewModel; constructor(options: Options) { super(options); @@ -92,75 +91,58 @@ export class CallViewModel extends ViewModel { return this.call.id; } - get errorViewModel(): ErrorViewModel | undefined { - return this._errorViewModel; - } - private get call(): GroupCall { return this.getOption("call"); } private onUpdate() { if (this.call.error) { - this._reportError(this.call.error); + this.reportError(this.call.error); } } async hangup() { - try { + this.logAndCatch("Call.hangup", async log => { if (this.call.hasJoined) { await this.call.leave(); } - } catch (err) { - this._reportError(err); - } + }); } async toggleCamera() { - const {localMedia, muteSettings} = this.call; - if (muteSettings && localMedia) { - // unmute but no track? - if (muteSettings.camera && !getStreamVideoTrack(localMedia.userMedia)) { - const stream = await this.platform.mediaDevices.getMediaTracks(!muteSettings.microphone, true); - await this.call.setMedia(localMedia.withUserMedia(stream)); - } else { - await this.call.setMuted(muteSettings.toggleCamera()); + this.logAndCatch("Call.toggleCamera", async log => { + const {localMedia, muteSettings} = this.call; + if (muteSettings && localMedia) { + // unmute but no track? + if (muteSettings.camera && !getStreamVideoTrack(localMedia.userMedia)) { + const stream = await this.platform.mediaDevices.getMediaTracks(!muteSettings.microphone, true); + await this.call.setMedia(localMedia.withUserMedia(stream)); + } else { + await this.call.setMuted(muteSettings.toggleCamera()); + } + this.emitChange(); } - this.emitChange(); - } + }); } async toggleMicrophone() { - const {localMedia, muteSettings} = this.call; - if (muteSettings && localMedia) { - // unmute but no track? - if (muteSettings.microphone && !getStreamAudioTrack(localMedia.userMedia)) { - const stream = await this.platform.mediaDevices.getMediaTracks(true, !muteSettings.camera); - await this.call.setMedia(localMedia.withUserMedia(stream)); - } else { - await this.call.setMuted(muteSettings.toggleMicrophone()); + this.logAndCatch("Call.toggleMicrophone", async log => { + const {localMedia, muteSettings} = this.call; + if (muteSettings && localMedia) { + // unmute but no track? + if (muteSettings.microphone && !getStreamAudioTrack(localMedia.userMedia)) { + const stream = await this.platform.mediaDevices.getMediaTracks(true, !muteSettings.camera); + await this.call.setMedia(localMedia.withUserMedia(stream)); + } else { + await this.call.setMuted(muteSettings.toggleMicrophone()); + } + this.emitChange(); } - this.emitChange(); - } - } - - private _reportError(error: Error) { - if (this._errorViewModel?.error === error) { - return; - } - this.disposeTracked(this._errorViewModel); - this._errorViewModel = new ErrorViewModel(this.childOptions({ - error, - onClose: () => { - this._errorViewModel = this.disposeTracked(this._errorViewModel); - this.emitChange("errorViewModel"); - } - })); - this.emitChange("errorViewModel"); + }); } } -class OwnMemberViewModel extends ViewModel implements IStreamViewModel { +class OwnMemberViewModel extends ErrorReportViewModel implements IStreamViewModel { private memberObservable: undefined | BaseObservableValue; constructor(options: Options) { @@ -233,12 +215,9 @@ class OwnMemberViewModel extends ViewModel implements IStreamViewModel type MemberOptions = BaseOptions & { member: Member, mediaRepository: MediaRepository, - session: Session }; -export class CallMemberViewModel extends ViewModel implements IStreamViewModel { - private _errorViewModel?: ErrorViewModel; - +export class CallMemberViewModel extends ErrorReportViewModel implements IStreamViewModel { get stream(): Stream | undefined { return this.member.remoteMedia?.userMedia; } @@ -247,10 +226,6 @@ export class CallMemberViewModel extends ViewModel implements ISt return this.getOption("member"); } - get errorViewModel(): ErrorViewModel | undefined { - return this._errorViewModel; - } - get isCameraMuted(): boolean { return this.member.remoteMuteSettings?.camera ?? true; } @@ -282,16 +257,8 @@ export class CallMemberViewModel extends ViewModel implements ISt } private mapMemberSyncErrorIfNeeded() { - if (this.member.error && (!this._errorViewModel || this._errorViewModel.error !== this.member.error)) { - this.disposeTracked(this._errorViewModel); - this._errorViewModel = this.track(new ErrorViewModel(this.childOptions({ - error: this.member.error, - onClose: () => { - this._errorViewModel = this.disposeTracked(this._errorViewModel); - this.emitChange("errorViewModel"); - }, - }))); - this.emitChange("errorViewModel"); + if (this.member.error) { + this.reportError(this.member.error); } } diff --git a/src/domain/session/room/RoomViewModel.js b/src/domain/session/room/RoomViewModel.js index d744e5d5..a5dfdc8f 100644 --- a/src/domain/session/room/RoomViewModel.js +++ b/src/domain/session/room/RoomViewModel.js @@ -20,7 +20,7 @@ import {ComposerViewModel} from "./ComposerViewModel.js" import {CallViewModel} from "./CallViewModel" import {PickMapObservableValue} from "../../../observable/value/PickMapObservableValue"; import {avatarInitials, getIdentifierColorNumber, getAvatarHttpUrl} from "../../avatar"; -import {ViewModel} from "../../ViewModel"; +import {ErrorReportViewModel} from "../../ErrorReportViewModel"; import {imageToInfo} from "../common.js"; import {LocalMedia} from "../../../matrix/calls/LocalMedia"; import { ErrorViewModel } from "../../ErrorViewModel"; @@ -28,7 +28,7 @@ import { ErrorViewModel } from "../../ErrorViewModel"; // this is a breaking SDK change though to make this option mandatory import {tileClassForEntry as defaultTileClassForEntry} from "./timeline/tiles/index"; -export class RoomViewModel extends ViewModel { +export class RoomViewModel extends ErrorReportViewModel { constructor(options) { super(options); const {room, tileClassForEntry} = options; @@ -37,7 +37,6 @@ export class RoomViewModel extends ViewModel { this._tileClassForEntry = tileClassForEntry ?? defaultTileClassForEntry; this._tileOptions = undefined; this._onRoomChange = this._onRoomChange.bind(this); - this._errorViewModel = null; this._composerVM = null; if (room.isArchived) { this._composerVM = new ArchivedViewModel(this.childOptions({archivedRoom: room})); @@ -73,14 +72,6 @@ export class RoomViewModel extends ViewModel { } } - _reportError(error) { - this._errorViewModel = new ErrorViewModel(this.childOptions({error, onClose: () => { - this._errorViewModel = null; - this.emitChange("errorViewModel"); - }})); - this.emitChange("errorViewModel"); - } - async load() { this._room.on("change", this._onRoomChange); try { @@ -97,7 +88,7 @@ export class RoomViewModel extends ViewModel { }))); this.emitChange("timelineViewModel"); } catch (error) { - this._reportError(error); + this.reportError(error); } this._clearUnreadAfterDelay(); } @@ -216,7 +207,7 @@ export class RoomViewModel extends ViewModel { await this._room.sendEvent("m.room.message", {msgtype, body: message}); } } catch (error) { - this._reportError(error); + this.reportError(error); return false; } return true; @@ -287,7 +278,7 @@ export class RoomViewModel extends ViewModel { this._room.createAttachment(thumbnail.blob, file.name); await this._room.sendEvent("m.room.message", content, attachments); } catch (error) { - this._reportError(error); + this.reportError(error); } } @@ -327,7 +318,7 @@ export class RoomViewModel extends ViewModel { } await this._room.sendEvent("m.room.message", content, attachments); } catch (error) { - this._reportError(error); + this.reportError(error); } } @@ -362,7 +353,7 @@ export class RoomViewModel extends ViewModel { const stream = await this.platform.mediaDevices.getMediaTracks(false, true); localMedia = new LocalMedia().withUserMedia(stream); } catch (err) { - this._reportError(new Error(`Could not get local audio and/or video stream: ${err.message}`)); + this.reportError(new Error(`Could not get local audio and/or video stream: ${err.message}`)); return; } const session = this.getOption("session"); @@ -371,13 +362,13 @@ export class RoomViewModel extends ViewModel { // this will set the callViewModel above as a call will be added to callHandler.calls call = await session.callHandler.createCall(this._room.id, "m.video", "A call " + Math.round(this.platform.random() * 100)); } catch (err) { - this._reportError(new Error(`Could not create call: ${err.message}`)); + this.reportError(new Error(`Could not create call: ${err.message}`)); return; } try { await call.join(localMedia); } catch (err) { - this._reportError(new Error(`Could not join call: ${err.message}`)); + this.reportError(new Error(`Could not join call: ${err.message}`)); return; } } diff --git a/src/domain/session/room/timeline/tiles/CallTile.js b/src/domain/session/room/timeline/tiles/CallTile.js index a3008cb9..44c39eef 100644 --- a/src/domain/session/room/timeline/tiles/CallTile.js +++ b/src/domain/session/room/timeline/tiles/CallTile.js @@ -28,7 +28,6 @@ export class CallTile extends SimpleTile { const calls = this.getOption("session").callHandler.calls; this._call = calls.get(this._entry.stateKey); this._callSubscription = undefined; - this._errorViewModel = undefined; if (this._call) { this._callSubscription = this._call.disposableOn("change", () => { // unsubscribe when terminated @@ -61,10 +60,6 @@ export class CallTile extends SimpleTile { return this._call && this._call.hasJoined; } - get errorViewModel() { - return this._errorViewModel; - } - get label() { if (this._call) { if (this._call.hasJoined) { @@ -78,25 +73,21 @@ export class CallTile extends SimpleTile { } async join() { - if (this.canJoin) { - try { + await this.logAndCatch("Call.join", async log => { + if (this.canJoin) { const stream = await this.platform.mediaDevices.getMediaTracks(false, true); const localMedia = new LocalMedia().withUserMedia(stream); await this._call.join(localMedia); - } catch (error) { - this._reportError(error); } - } + }); } async leave() { - if (this.canLeave) { - try { - this._call.leave(); - } catch (err) { - this._reportError(err); + await this.logAndCatch("Call.leave", async log => { + if (this.canLeave) { + await this._call.leave(); } - } + }); } dispose() { @@ -104,12 +95,4 @@ export class CallTile extends SimpleTile { this._callSubscription = this._callSubscription(); } } - - _reportError(error) { - this._errorViewModel = new ErrorViewModel(this.childOptions({error, onClose: () => { - this._errorViewModel = undefined; - this.emitChange("errorViewModel"); - }})); - this.emitChange("errorViewModel"); - } } diff --git a/src/domain/session/room/timeline/tiles/SimpleTile.js b/src/domain/session/room/timeline/tiles/SimpleTile.js index 18e6ba17..aa26da81 100644 --- a/src/domain/session/room/timeline/tiles/SimpleTile.js +++ b/src/domain/session/room/timeline/tiles/SimpleTile.js @@ -15,10 +15,10 @@ limitations under the License. */ import {UpdateAction} from "../UpdateAction.js"; -import {ViewModel} from "../../../../ViewModel"; +import {ErrorReportViewModel} from "../../../../ErrorReportViewModel"; import {SendStatus} from "../../../../../matrix/room/sending/PendingEvent.js"; -export class SimpleTile extends ViewModel { +export class SimpleTile extends ErrorReportViewModel { constructor(entry, options) { super(options); this._entry = entry; From 80be2b74570df5e4d78cdb160ae5271ab07a5eb0 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Fri, 13 Jan 2023 14:20:29 +0100 Subject: [PATCH 16/32] fix missing import --- src/domain/session/room/RoomViewModel.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/domain/session/room/RoomViewModel.js b/src/domain/session/room/RoomViewModel.js index a5dfdc8f..c864222e 100644 --- a/src/domain/session/room/RoomViewModel.js +++ b/src/domain/session/room/RoomViewModel.js @@ -21,6 +21,7 @@ import {CallViewModel} from "./CallViewModel" import {PickMapObservableValue} from "../../../observable/value/PickMapObservableValue"; import {avatarInitials, getIdentifierColorNumber, getAvatarHttpUrl} from "../../avatar"; import {ErrorReportViewModel} from "../../ErrorReportViewModel"; +import {ViewModel} from "../../ViewModel"; import {imageToInfo} from "../common.js"; import {LocalMedia} from "../../../matrix/calls/LocalMedia"; import { ErrorViewModel } from "../../ErrorViewModel"; From 29a7b0451e9b6f4167ae36298841677dd87a6a5f Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Fri, 13 Jan 2023 14:20:33 +0100 Subject: [PATCH 17/32] prevent errors in promises from being uncaught by returning a promise that has the error swallowed --- src/domain/ErrorReportViewModel.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/domain/ErrorReportViewModel.ts b/src/domain/ErrorReportViewModel.ts index eb6ed0aa..68fe580c 100644 --- a/src/domain/ErrorReportViewModel.ts +++ b/src/domain/ErrorReportViewModel.ts @@ -49,9 +49,12 @@ export class ErrorReportViewModel extends ViewModel protected logAndCatch(labelOrValues: LabelOrValues, callback: LogCallback, errorValue: T = undefined as unknown as T): T { try { - const result = this.logger.run(labelOrValues, callback); + let result = this.logger.run(labelOrValues, callback); if (result instanceof Promise) { - result.catch(err => this.reportError(err)); + result = result.catch(err => { + this.reportError(err); + return errorValue; + }) as unknown as T; } return result; } catch (err) { From cec9f6b691b7473c98d6cbaf8cb2b86cf8494e70 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Fri, 13 Jan 2023 21:26:01 +0100 Subject: [PATCH 18/32] WIP for docs on error handling --- doc/error-handling.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 doc/error-handling.md diff --git a/doc/error-handling.md b/doc/error-handling.md new file mode 100644 index 00000000..5e136735 --- /dev/null +++ b/doc/error-handling.md @@ -0,0 +1,13 @@ +# Error handling + +Ideally, every error that is unexpected and can't be automatically recovered from without degrading the experience is shown in the UI. This is the task of the view model, and you can use `ErrorReportViewModel`, a dedicated base view model class for this purpose. It exposes a child view model, `ErrorViewModel`, when `reportError` is called which can be paired with `ErrorView` in the view to present an error message from which debug logs can also be sent. + +Methods on classes from the `matrix` layer can often throw errors and those errors should be caught in the view model and reported with `reportError`. As a convenience method, there is also `logAndCatch` which calls a callback within a log item and also a try catch that reports the error. + +## Sync errors + +There are some errors that are throw during background processes though, most notably the sync loop. These processes are not triggered by the view model directly, and hence there is not always a method call they can wrap in a try/catch. For this, there is the `ErrorBoundary` utility class. Since almost all aspects of the client can be updated through the sync loop, it is not too helpful if there is only one try/catch around the whole sync and we stop sync if something goes wrong. + +Instead, it's more helpful to split up the error handling into different scopes, where errors are stored and not rethrown when leaving the scope. One example is to have a scope per room. In this way, we can isolate an error occuring during sync to a specific room, and report it in the UI of that room. + +There is an extra complication though. The `writeSync` sync lifecycle step should not swallow any errors, or data loss can occur. This is because the whole `writeSync` lifecycle step is writes all changes (for all rooms, the session, ...) for a sync response in one transaction. This includes the sync token. So if there is an error in `writeSync` of a given room preventing storing all changes the sync response would cause, \ No newline at end of file From f853871722254a460855369ed2cc19a8ee868cb0 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Mon, 16 Jan 2023 10:11:04 +0100 Subject: [PATCH 19/32] some rewording on error handling doc --- doc/error-handling.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/error-handling.md b/doc/error-handling.md index 5e136735..21c67ee5 100644 --- a/doc/error-handling.md +++ b/doc/error-handling.md @@ -1,13 +1,13 @@ # Error handling -Ideally, every error that is unexpected and can't be automatically recovered from without degrading the experience is shown in the UI. This is the task of the view model, and you can use `ErrorReportViewModel`, a dedicated base view model class for this purpose. It exposes a child view model, `ErrorViewModel`, when `reportError` is called which can be paired with `ErrorView` in the view to present an error message from which debug logs can also be sent. +Ideally, every error that is unexpected and can't be automatically recovered from without degrading the experience is shown in the UI. This is the task of the view model, and you can use `ErrorReportViewModel` for this purpose, a dedicated base view model class. It exposes a child view model, `ErrorViewModel`, when `reportError` is called which can be paired with `ErrorView` in the view to present an error message from which debug logs can also be sent. -Methods on classes from the `matrix` layer can often throw errors and those errors should be caught in the view model and reported with `reportError`. As a convenience method, there is also `logAndCatch` which calls a callback within a log item and also a try catch that reports the error. +Methods on classes from the `matrix` layer can often throw errors and those errors should be caught in the view model and reported with `reportError`. As a convenience method, there is also `logAndCatch` when inheriting from `ErrorReportViewModel` which combines logging and error reporting; it calls a callback within a log item and also a try catch that reports the error. -## Sync errors +## Sync errors & ErrorBoundary -There are some errors that are throw during background processes though, most notably the sync loop. These processes are not triggered by the view model directly, and hence there is not always a method call they can wrap in a try/catch. For this, there is the `ErrorBoundary` utility class. Since almost all aspects of the client can be updated through the sync loop, it is not too helpful if there is only one try/catch around the whole sync and we stop sync if something goes wrong. +There are some errors that are thrown during background processes though, most notably the sync loop. These processes are not triggered by the view model directly, and hence there is not always a method call they can wrap in a try/catch. For this, there is the `ErrorBoundary` utility class. Since almost all aspects of the client can be updated through the sync loop, it is also not too helpful if there is only one try/catch around the whole sync and we stop sync if something goes wrong. Instead, it's more helpful to split up the error handling into different scopes, where errors are stored and not rethrown when leaving the scope. One example is to have a scope per room. In this way, we can isolate an error occuring during sync to a specific room, and report it in the UI of that room. -There is an extra complication though. The `writeSync` sync lifecycle step should not swallow any errors, or data loss can occur. This is because the whole `writeSync` lifecycle step is writes all changes (for all rooms, the session, ...) for a sync response in one transaction. This includes the sync token. So if there is an error in `writeSync` of a given room preventing storing all changes the sync response would cause, \ No newline at end of file +There is an extra complication though. The `writeSync` sync lifecycle step should not swallow any errors, or data loss can occur. This is because the whole `writeSync` lifecycle step writes all changes (for all rooms, the session, ...) for a sync response in one transaction (including the sync token), and aborts the transaction and stops sync if there is an error thrown during this step. So if there is an error in `writeSync` of a given room, it's fair to assume not all changes it was planning to write were passed to the transaction, as it got interrupted by the exception. Therefore, if we would swallow the error, data loss can occur as we'd not get another chance to write these changes to disk as we would have advanced the sync token. Therefore, code in the `writeSync` lifecycle step should be written defensively but always throw. From 1de92af2eb4622ae14a7b51816e1db99add2f3c5 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 17 Jan 2023 08:25:08 +0000 Subject: [PATCH 20/32] Update src/domain/session/room/CallViewModel.ts Co-authored-by: R Midhun Suresh --- src/domain/session/room/CallViewModel.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/domain/session/room/CallViewModel.ts b/src/domain/session/room/CallViewModel.ts index ee0ad502..98473888 100644 --- a/src/domain/session/room/CallViewModel.ts +++ b/src/domain/session/room/CallViewModel.ts @@ -21,7 +21,7 @@ import {getStreamVideoTrack, getStreamAudioTrack} from "../../../matrix/calls/co import {avatarInitials, getIdentifierColorNumber, getAvatarHttpUrl} from "../../avatar"; import {EventObservableValue} from "../../../observable/value/EventObservableValue"; import {ObservableValueMap} from "../../../observable/map/ObservableValueMap"; -import { ErrorViewModel } from "../../ErrorViewModel"; +import {ErrorViewModel} from "../../ErrorViewModel"; import type {Room} from "../../../matrix/room/Room"; import type {GroupCall} from "../../../matrix/calls/group/GroupCall"; import type {Member} from "../../../matrix/calls/group/Member"; From 7d80fbda4cadc14b96d4a46f7958f747d79fb2e7 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 17 Jan 2023 08:25:16 +0000 Subject: [PATCH 21/32] Update src/domain/session/room/CallViewModel.ts Co-authored-by: R Midhun Suresh --- src/domain/session/room/CallViewModel.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/domain/session/room/CallViewModel.ts b/src/domain/session/room/CallViewModel.ts index 98473888..df2da3c6 100644 --- a/src/domain/session/room/CallViewModel.ts +++ b/src/domain/session/room/CallViewModel.ts @@ -30,7 +30,7 @@ import type {BaseObservableList} from "../../../observable/list/BaseObservableLi import type {BaseObservableValue} from "../../../observable/value/BaseObservableValue"; import type {Stream} from "../../../platform/types/MediaDevices"; import type {MediaRepository} from "../../../matrix/net/MediaRepository"; -import type { Session } from "../../../matrix/Session"; +import type {Session} from "../../../matrix/Session"; type Options = BaseOptions & { call: GroupCall, From f421cdd4f2e2c400c243b683ce2a623b5219010b Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 17 Jan 2023 08:25:33 +0000 Subject: [PATCH 22/32] Update src/domain/session/room/RoomViewModel.js Co-authored-by: R Midhun Suresh --- src/domain/session/room/RoomViewModel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/domain/session/room/RoomViewModel.js b/src/domain/session/room/RoomViewModel.js index c864222e..421041e9 100644 --- a/src/domain/session/room/RoomViewModel.js +++ b/src/domain/session/room/RoomViewModel.js @@ -24,7 +24,7 @@ import {ErrorReportViewModel} from "../../ErrorReportViewModel"; import {ViewModel} from "../../ViewModel"; import {imageToInfo} from "../common.js"; import {LocalMedia} from "../../../matrix/calls/LocalMedia"; -import { ErrorViewModel } from "../../ErrorViewModel"; +import {ErrorViewModel} from "../../ErrorViewModel"; // TODO: remove fallback so default isn't included in bundle for SDK users that have their custom tileClassForEntry // this is a breaking SDK change though to make this option mandatory import {tileClassForEntry as defaultTileClassForEntry} from "./timeline/tiles/index"; From cc653884a515e04d38d37f814b9447f38659fb06 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 17 Jan 2023 09:28:23 +0100 Subject: [PATCH 23/32] remove getter that is now in parent class --- src/domain/session/room/RoomViewModel.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/domain/session/room/RoomViewModel.js b/src/domain/session/room/RoomViewModel.js index 421041e9..0c815949 100644 --- a/src/domain/session/room/RoomViewModel.js +++ b/src/domain/session/room/RoomViewModel.js @@ -141,10 +141,6 @@ export class RoomViewModel extends ErrorReportViewModel { get timelineViewModel() { return this._timelineVM; } get isEncrypted() { return this._room.isEncrypted; } - get errorViewModel() { - return this._errorViewModel; - } - get avatarLetter() { return avatarInitials(this.name); } From a2c44484b21a96dfcc47121fd2af936a08b29cb2 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 17 Jan 2023 09:29:03 +0100 Subject: [PATCH 24/32] newline --- src/domain/rageshake.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/domain/rageshake.ts b/src/domain/rageshake.ts index 08e82d8c..cc8aec07 100644 --- a/src/domain/rageshake.ts +++ b/src/domain/rageshake.ts @@ -91,4 +91,4 @@ export async function submitLogsFromSessionToDefaultServer(session: Session, pla bugReportEndpointUrl, platform.request ); -} \ No newline at end of file +} From 0dbb7d4e50d7f947cd1d2b4e978c91b988a16c9d Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 17 Jan 2023 11:37:49 +0100 Subject: [PATCH 25/32] use logAndCatch in RoomViewModel, everything reporting errors also logs --- src/domain/session/room/RoomViewModel.js | 127 +++++++++++------------ 1 file changed, 61 insertions(+), 66 deletions(-) diff --git a/src/domain/session/room/RoomViewModel.js b/src/domain/session/room/RoomViewModel.js index 0c815949..a3542ebb 100644 --- a/src/domain/session/room/RoomViewModel.js +++ b/src/domain/session/room/RoomViewModel.js @@ -74,9 +74,9 @@ export class RoomViewModel extends ErrorReportViewModel { } async load() { - this._room.on("change", this._onRoomChange); - try { - const timeline = await this._room.openTimeline(); + this.logAndCatch("Room.load", async log => { + this._room.on("change", this._onRoomChange); + const timeline = await this._room.openTimeline(log); this._tileOptions = this.childOptions({ session: this.getOption("session"), roomVM: this, @@ -88,13 +88,11 @@ export class RoomViewModel extends ErrorReportViewModel { timeline, }))); this.emitChange("timelineViewModel"); - } catch (error) { - this.reportError(error); - } - this._clearUnreadAfterDelay(); + await this._clearUnreadAfterDelay(log); + }); } - async _clearUnreadAfterDelay() { + async _clearUnreadAfterDelay(log) { if (this._room.isArchived || this._clearUnreadTimout) { return; } @@ -104,7 +102,9 @@ export class RoomViewModel extends ErrorReportViewModel { await this._room.clearUnread(); this._clearUnreadTimout = null; } catch (err) { - if (err.name !== "AbortError") { + if (err.name === "AbortError") { + log.set("clearUnreadCancelled", true); + } else { throw err; } } @@ -190,62 +190,61 @@ export class RoomViewModel extends ErrorReportViewModel { } } - async _sendMessage(message, replyingTo) { - if (!this._room.isArchived && message) { - try { + _sendMessage(message, replyingTo) { + return this.logAndCatch("sendMessage", async log => { + let success = false; + if (!this._room.isArchived && message) { let msgtype = "m.text"; if (message.startsWith("/me ")) { message = message.substr(4).trim(); msgtype = "m.emote"; } if (replyingTo) { + log.set("replyingTo", replyingTo.eventId); + // TODO: reply should not send? it should just return the content for the reply and we send it ourselves await replyingTo.reply(msgtype, message); } else { - await this._room.sendEvent("m.room.message", {msgtype, body: message}); + await this._room.sendEvent("m.room.message", {msgtype, body: message}, undefined, log); } - } catch (error) { - this.reportError(error); - return false; + success = true; } - return true; - } - return false; + log.set("success", success); + return success; + }, false); } - async _pickAndSendFile() { - try { + _pickAndSendFile() { + return this.logAndCatch("sendFile", async log => { const file = await this.platform.openFile(); if (!file) { + log.set("cancelled", true); return; } - return this._sendFile(file); - } catch (err) { - console.error(err); - } + return this._sendFile(file, log); + }); } - async _sendFile(file) { + async _sendFile(file, log) { const content = { body: file.name, msgtype: "m.file" }; await this._room.sendEvent("m.room.message", content, { "url": this._room.createAttachment(file.blob, file.name) - }); + }, log); } - async _pickAndSendVideo() { - try { + _pickAndSendVideo() { + return this.logAndCatch("sendVideo", async log => { if (!this.platform.hasReadPixelPermission()) { - alert("Please allow canvas image data access, so we can scale your images down."); - return; + throw new Error("Please allow canvas image data access, so we can scale your images down."); } const file = await this.platform.openFile("video/*"); if (!file) { return; } if (!file.blob.mimeType.startsWith("video/")) { - return this._sendFile(file); + return this._sendFile(file, log); } let video; try { @@ -273,24 +272,23 @@ export class RoomViewModel extends ErrorReportViewModel { content.info.thumbnail_info = imageToInfo(thumbnail); attachments["info.thumbnail_url"] = this._room.createAttachment(thumbnail.blob, file.name); - await this._room.sendEvent("m.room.message", content, attachments); - } catch (error) { - this.reportError(error); - } + await this._room.sendEvent("m.room.message", content, attachments, log); + }); } async _pickAndSendPicture() { - try { + this.logAndCatch("sendPicture", async log => { if (!this.platform.hasReadPixelPermission()) { alert("Please allow canvas image data access, so we can scale your images down."); return; } const file = await this.platform.openFile("image/*"); if (!file) { + log.set("cancelled", true); return; } if (!file.blob.mimeType.startsWith("image/")) { - return this._sendFile(file); + return this._sendFile(file, log); } let image = await this.platform.loadImage(file.blob); const limit = await this.platform.settingsStorage.getInt("sentImageSizeLimit"); @@ -313,10 +311,8 @@ export class RoomViewModel extends ErrorReportViewModel { attachments["info.thumbnail_url"] = this._room.createAttachment(thumbnail.blob, file.name); } - await this._room.sendEvent("m.room.message", content, attachments); - } catch (error) { - this.reportError(error); - } + await this._room.sendEvent("m.room.message", content, attachments, log); + }); } get room() { @@ -344,30 +340,29 @@ export class RoomViewModel extends ErrorReportViewModel { } } - async startCall() { - let localMedia; - try { - const stream = await this.platform.mediaDevices.getMediaTracks(false, true); - localMedia = new LocalMedia().withUserMedia(stream); - } catch (err) { - this.reportError(new Error(`Could not get local audio and/or video stream: ${err.message}`)); - return; - } - const session = this.getOption("session"); - let call; - try { - // this will set the callViewModel above as a call will be added to callHandler.calls - call = await session.callHandler.createCall(this._room.id, "m.video", "A call " + Math.round(this.platform.random() * 100)); - } catch (err) { - this.reportError(new Error(`Could not create call: ${err.message}`)); - return; - } - try { - await call.join(localMedia); - } catch (err) { - this.reportError(new Error(`Could not join call: ${err.message}`)); - return; - } + startCall() { + return this.logAndCatch("startCall", async log => { + let localMedia; + try { + const stream = await this.platform.mediaDevices.getMediaTracks(false, true); + localMedia = new LocalMedia().withUserMedia(stream); + } catch (err) { + throw new Error(`Could not get local audio and/or video stream: ${err.message}`); + } + const session = this.getOption("session"); + let call; + try { + // this will set the callViewModel above as a call will be added to callHandler.calls + call = await session.callHandler.createCall(this._room.id, "m.video", "A call " + Math.round(this.platform.random() * 100)); + } catch (err) { + throw new Error(`Could not create call: ${err.message}`); + } + try { + await call.join(localMedia); + } catch (err) { + throw new Error(`Could not join call: ${err.message}`); + } + }); } } From e33209b747140c51d83a585e01358251adb70f5a Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 17 Jan 2023 12:23:20 +0100 Subject: [PATCH 26/32] start logging in view model and pass it on to model methods (calls+room) --- src/domain/session/room/CallViewModel.ts | 4 +- src/domain/session/room/RoomViewModel.js | 30 ++-- .../session/room/timeline/tiles/CallTile.js | 9 +- src/matrix/calls/CallHandler.ts | 141 ++++++++++-------- src/matrix/calls/group/GroupCall.ts | 95 ++++++------ 5 files changed, 151 insertions(+), 128 deletions(-) diff --git a/src/domain/session/room/CallViewModel.ts b/src/domain/session/room/CallViewModel.ts index df2da3c6..dd942878 100644 --- a/src/domain/session/room/CallViewModel.ts +++ b/src/domain/session/room/CallViewModel.ts @@ -102,9 +102,9 @@ export class CallViewModel extends ErrorReportViewModel { } async hangup() { - this.logAndCatch("Call.hangup", async log => { + this.logAndCatch("CallViewModel.hangup", async log => { if (this.call.hasJoined) { - await this.call.leave(); + await this.call.leave(log); } }); } diff --git a/src/domain/session/room/RoomViewModel.js b/src/domain/session/room/RoomViewModel.js index a3542ebb..5a29bd85 100644 --- a/src/domain/session/room/RoomViewModel.js +++ b/src/domain/session/room/RoomViewModel.js @@ -24,7 +24,6 @@ import {ErrorReportViewModel} from "../../ErrorReportViewModel"; import {ViewModel} from "../../ViewModel"; import {imageToInfo} from "../common.js"; import {LocalMedia} from "../../../matrix/calls/LocalMedia"; -import {ErrorViewModel} from "../../ErrorViewModel"; // TODO: remove fallback so default isn't included in bundle for SDK users that have their custom tileClassForEntry // this is a breaking SDK change though to make this option mandatory import {tileClassForEntry as defaultTileClassForEntry} from "./timeline/tiles/index"; @@ -74,7 +73,7 @@ export class RoomViewModel extends ErrorReportViewModel { } async load() { - this.logAndCatch("Room.load", async log => { + this.logAndCatch("RoomViewModel.load", async log => { this._room.on("change", this._onRoomChange); const timeline = await this._room.openTimeline(log); this._tileOptions = this.childOptions({ @@ -99,7 +98,7 @@ export class RoomViewModel extends ErrorReportViewModel { this._clearUnreadTimout = this.clock.createTimeout(2000); try { await this._clearUnreadTimout.elapsed(); - await this._room.clearUnread(); + await this._room.clearUnread(log); this._clearUnreadTimout = null; } catch (err) { if (err.name === "AbortError") { @@ -111,7 +110,9 @@ export class RoomViewModel extends ErrorReportViewModel { } focus() { - this._clearUnreadAfterDelay(); + this.logAndCatch("RoomViewModel.focus", async log => { + this._clearUnreadAfterDelay(log); + }); } dispose() { @@ -191,7 +192,7 @@ export class RoomViewModel extends ErrorReportViewModel { } _sendMessage(message, replyingTo) { - return this.logAndCatch("sendMessage", async log => { + return this.logAndCatch("RoomViewModel.sendMessage", async log => { let success = false; if (!this._room.isArchived && message) { let msgtype = "m.text"; @@ -214,7 +215,7 @@ export class RoomViewModel extends ErrorReportViewModel { } _pickAndSendFile() { - return this.logAndCatch("sendFile", async log => { + return this.logAndCatch("RoomViewModel.sendFile", async log => { const file = await this.platform.openFile(); if (!file) { log.set("cancelled", true); @@ -235,7 +236,7 @@ export class RoomViewModel extends ErrorReportViewModel { } _pickAndSendVideo() { - return this.logAndCatch("sendVideo", async log => { + return this.logAndCatch("RoomViewModel.sendVideo", async log => { if (!this.platform.hasReadPixelPermission()) { throw new Error("Please allow canvas image data access, so we can scale your images down."); } @@ -277,7 +278,7 @@ export class RoomViewModel extends ErrorReportViewModel { } async _pickAndSendPicture() { - this.logAndCatch("sendPicture", async log => { + this.logAndCatch("RoomViewModel.sendPicture", async log => { if (!this.platform.hasReadPixelPermission()) { alert("Please allow canvas image data access, so we can scale your images down."); return; @@ -341,7 +342,8 @@ export class RoomViewModel extends ErrorReportViewModel { } startCall() { - return this.logAndCatch("startCall", async log => { + return this.logAndCatch("RoomViewModel.startCall", async log => { + log.set("roomId", this._room.id); let localMedia; try { const stream = await this.platform.mediaDevices.getMediaTracks(false, true); @@ -353,12 +355,18 @@ export class RoomViewModel extends ErrorReportViewModel { let call; try { // this will set the callViewModel above as a call will be added to callHandler.calls - call = await session.callHandler.createCall(this._room.id, "m.video", "A call " + Math.round(this.platform.random() * 100)); + call = await session.callHandler.createCall( + this._room.id, + "m.video", + "A call " + Math.round(this.platform.random() * 100), + undefined, + log + ); } catch (err) { throw new Error(`Could not create call: ${err.message}`); } try { - await call.join(localMedia); + await call.join(localMedia, log); } catch (err) { throw new Error(`Could not join call: ${err.message}`); } diff --git a/src/domain/session/room/timeline/tiles/CallTile.js b/src/domain/session/room/timeline/tiles/CallTile.js index 44c39eef..c792496e 100644 --- a/src/domain/session/room/timeline/tiles/CallTile.js +++ b/src/domain/session/room/timeline/tiles/CallTile.js @@ -16,7 +16,6 @@ limitations under the License. import {SimpleTile} from "./SimpleTile.js"; import {LocalMedia} from "../../../../../matrix/calls/LocalMedia"; -import {ErrorViewModel} from "../../../../ErrorViewModel" // TODO: timeline entries for state events with the same state key and type // should also update previous entries in the timeline, so we can update the name of the call, whether it is terminated, etc ... @@ -73,19 +72,19 @@ export class CallTile extends SimpleTile { } async join() { - await this.logAndCatch("Call.join", async log => { + await this.logAndCatch("CallTile.join", async log => { if (this.canJoin) { const stream = await this.platform.mediaDevices.getMediaTracks(false, true); const localMedia = new LocalMedia().withUserMedia(stream); - await this._call.join(localMedia); + await this._call.join(localMedia, log); } }); } async leave() { - await this.logAndCatch("Call.leave", async log => { + await this.logAndCatch("CallTile.leave", async log => { if (this.canLeave) { - await this._call.leave(); + await this._call.leave(log); } }); } diff --git a/src/matrix/calls/CallHandler.ts b/src/matrix/calls/CallHandler.ts index 2b914a2f..51afcda9 100644 --- a/src/matrix/calls/CallHandler.ts +++ b/src/matrix/calls/CallHandler.ts @@ -65,16 +65,26 @@ export class CallHandler implements RoomStateHandler { }); } - async loadCalls(intent: CallIntent = CallIntent.Ring) { - const txn = await this._getLoadTxn(); - const callEntries = await txn.calls.getByIntent(intent); - this._loadCallEntries(callEntries, txn); + loadCalls(intent?: CallIntent, log?: ILogItem) { + return this.options.logger.wrapOrRun(log, "CallHandler.loadCalls", async log => { + if (!intent) { + intent = CallIntent.Ring; + } + log.set("intent", intent); + const txn = await this._getLoadTxn(); + const callEntries = await txn.calls.getByIntent(intent); + await this._loadCallEntries(callEntries, txn, log); + }); } - async loadCallsForRoom(intent: CallIntent, roomId: string) { - const txn = await this._getLoadTxn(); - const callEntries = await txn.calls.getByIntentAndRoom(intent, roomId); - this._loadCallEntries(callEntries, txn); + loadCallsForRoom(intent: CallIntent, roomId: string, log?: ILogItem) { + return this.options.logger.wrapOrRun(log, "CallHandler.loadCallsForRoom", async log => { + log.set("intent", intent); + log.set("roomId", roomId); + const txn = await this._getLoadTxn(); + const callEntries = await txn.calls.getByIntentAndRoom(intent, roomId); + await this._loadCallEntries(callEntries, txn, log); + }); } private async _getLoadTxn(): Promise { @@ -86,68 +96,71 @@ export class CallHandler implements RoomStateHandler { return txn; } - private async _loadCallEntries(callEntries: CallEntry[], txn: Transaction): Promise { - return this.options.logger.run({l: "loading calls", t: CALL_LOG_TYPE}, async log => { - log.set("entries", callEntries.length); - await Promise.all(callEntries.map(async callEntry => { - if (this._calls.get(callEntry.callId)) { - return; + private async _loadCallEntries(callEntries: CallEntry[], txn: Transaction, log: ILogItem): Promise { + log.set("entries", callEntries.length); + await Promise.all(callEntries.map(async callEntry => { + if (this._calls.get(callEntry.callId)) { + return; + } + const event = await txn.roomState.get(callEntry.roomId, EventType.GroupCall, callEntry.callId); + if (event) { + const call = new GroupCall(event.event.state_key, false, event.event.content, event.roomId, this.groupCallOptions); + this._calls.set(call.id, call); + } + })); + const roomIds = Array.from(new Set(callEntries.map(e => e.roomId))); + await Promise.all(roomIds.map(async roomId => { + // TODO: don't load all members until we need them + const callsMemberEvents = await txn.roomState.getAllForType(roomId, EventType.GroupCallMember); + await Promise.all(callsMemberEvents.map(async entry => { + const userId = entry.event.sender; + const roomMemberState = await txn.roomState.get(roomId, MEMBER_EVENT_TYPE, userId); + let roomMember; + if (roomMemberState) { + roomMember = RoomMember.fromMemberEvent(roomMemberState.event); } - const event = await txn.roomState.get(callEntry.roomId, EventType.GroupCall, callEntry.callId); - if (event) { - const call = new GroupCall(event.event.state_key, false, event.event.content, event.roomId, this.groupCallOptions); - this._calls.set(call.id, call); + if (!roomMember) { + // we'll be missing the member here if we received a call and it's members + // as pre-gap state and the members weren't active in the timeline we got. + roomMember = RoomMember.fromUserId(roomId, userId, "join"); } + this.handleCallMemberEvent(entry.event, roomMember, roomId, log); })); - const roomIds = Array.from(new Set(callEntries.map(e => e.roomId))); - await Promise.all(roomIds.map(async roomId => { - // TODO: don't load all members until we need them - const callsMemberEvents = await txn.roomState.getAllForType(roomId, EventType.GroupCallMember); - await Promise.all(callsMemberEvents.map(async entry => { - const userId = entry.event.sender; - const roomMemberState = await txn.roomState.get(roomId, MEMBER_EVENT_TYPE, userId); - let roomMember; - if (roomMemberState) { - roomMember = RoomMember.fromMemberEvent(roomMemberState.event); - } - if (!roomMember) { - // we'll be missing the member here if we received a call and it's members - // as pre-gap state and the members weren't active in the timeline we got. - roomMember = RoomMember.fromUserId(roomId, userId, "join"); - } - this.handleCallMemberEvent(entry.event, roomMember, roomId, log); - })); - })); - log.set("newSize", this._calls.size); - }); + })); + log.set("newSize", this._calls.size); } - async createCall(roomId: string, type: "m.video" | "m.voice", name: string, intent: CallIntent = CallIntent.Ring): Promise { - const call = new GroupCall(makeId("conf-"), true, { - "m.name": name, - "m.intent": intent - }, roomId, this.groupCallOptions); - this._calls.set(call.id, call); + createCall(roomId: string, type: "m.video" | "m.voice", name: string, intent?: CallIntent, log?: ILogItem): Promise { + return this.options.logger.wrapOrRun(log, "CallHandler.createCall", async log => { + if (!intent) { + intent = CallIntent.Ring; + } + const call = new GroupCall(makeId("conf-"), true, { + "m.name": name, + "m.intent": intent + }, roomId, this.groupCallOptions); + this._calls.set(call.id, call); - try { - await call.create(type); - // store call info so it will ring again when reopening the app - const txn = await this.options.storage.readWriteTxn([this.options.storage.storeNames.calls]); - txn.calls.add({ - intent: call.intent, - callId: call.id, - timestamp: this.options.clock.now(), - roomId: roomId - }); - await txn.complete(); - } catch (err) { - //if (err.name === "ConnectionError") { - // if we're offline, give up and remove the call again - this._calls.remove(call.id); - //} - throw err; - } - return call; + try { + await call.create(type, log); + // store call info so it will ring again when reopening the app + const txn = await this.options.storage.readWriteTxn([this.options.storage.storeNames.calls]); + txn.calls.add({ + intent: call.intent, + callId: call.id, + timestamp: this.options.clock.now(), + roomId: roomId + }); + await txn.complete(); + } catch (err) { + //if (err.name === "ConnectionError") { + // if we're offline, give up and remove the call again + this._calls.remove(call.id); + //} + throw err; + } + return call; + }); } get calls(): BaseObservableMap { return this._calls; } diff --git a/src/matrix/calls/group/GroupCall.ts b/src/matrix/calls/group/GroupCall.ts index 53a87d5f..d700f2a0 100644 --- a/src/matrix/calls/group/GroupCall.ts +++ b/src/matrix/calls/group/GroupCall.ts @@ -162,45 +162,48 @@ export class GroupCall extends EventEmitter<{change: never}> { return this.errorBoundary.error; } - async join(localMedia: LocalMedia): Promise { - if (this._state !== GroupCallState.Created || this.joinedData) { - return; - } - const logItem = this.options.logger.child({ - l: "answer call", - t: CALL_LOG_TYPE, - id: this.id, - ownSessionId: this.options.sessionId - }); - const turnServer = await this.options.turnServerSource.getSettings(logItem); - const membersLogItem = logItem.child("member connections"); - const localMuteSettings = new MuteSettings(); - localMuteSettings.updateTrackInfo(localMedia.userMedia); - const localPreviewMedia = localMedia.asPreview(); - const joinedData = new JoinedData( - logItem, - membersLogItem, - localMedia, - localPreviewMedia, - localMuteSettings, - turnServer - ); - this.joinedData = joinedData; - await joinedData.logItem.wrap("join", async log => { - this._state = GroupCallState.Joining; - this.emitChange(); - await log.wrap("update member state", async log => { - const memberContent = await this._createMemberPayload(true); - log.set("payload", memberContent); - // send m.call.member state event - const request = this.options.hsApi.sendState(this.roomId, EventType.GroupCallMember, this.options.ownUserId, memberContent, {log}); - await request.response(); - this.emitChange(); - }); - // send invite to all members that are < my userId - for (const [,member] of this._members) { - this.connectToMember(member, joinedData, log); + join(localMedia: LocalMedia, log?: ILogItem): Promise { + return this.options.logger.wrapOrRun(log, "Call.join", async joinLog => { + if (this._state !== GroupCallState.Created || this.joinedData) { + return; } + const logItem = this.options.logger.child({ + l: "Call.connection", + t: CALL_LOG_TYPE, + id: this.id, + ownSessionId: this.options.sessionId + }); + const turnServer = await this.options.turnServerSource.getSettings(logItem); + const membersLogItem = logItem.child("member connections"); + const localMuteSettings = new MuteSettings(); + localMuteSettings.updateTrackInfo(localMedia.userMedia); + const localPreviewMedia = localMedia.asPreview(); + const joinedData = new JoinedData( + logItem, + membersLogItem, + localMedia, + localPreviewMedia, + localMuteSettings, + turnServer + ); + this.joinedData = joinedData; + await joinedData.logItem.wrap("join", async log => { + joinLog.refDetached(log); + this._state = GroupCallState.Joining; + this.emitChange(); + await log.wrap("update member state", async log => { + const memberContent = await this._createMemberPayload(true); + log.set("payload", memberContent); + // send m.call.member state event + const request = this.options.hsApi.sendState(this.roomId, EventType.GroupCallMember, this.options.ownUserId, memberContent, {log}); + await request.response(); + this.emitChange(); + }); + // send invite to all members that are < my userId + for (const [,member] of this._members) { + this.connectToMember(member, joinedData, log); + } + }); }); } @@ -263,12 +266,12 @@ export class GroupCall extends EventEmitter<{change: never}> { return this._state === GroupCallState.Joining || this._state === GroupCallState.Joined; } - async leave(): Promise { - const {joinedData} = this; - if (!joinedData) { - return; - } - await joinedData.logItem.wrap("leave", async log => { + async leave(log?: ILogItem): Promise { + await this.options.logger.wrapOrRun(log, "Call.leave", async log => { + const {joinedData} = this; + if (!joinedData) { + return; + } try { joinedData.renewMembershipTimeout?.dispose(); joinedData.renewMembershipTimeout = undefined; @@ -310,8 +313,8 @@ export class GroupCall extends EventEmitter<{change: never}> { } /** @internal */ - create(type: "m.video" | "m.voice", log?: ILogItem): Promise { - return this.options.logger.wrapOrRun(log, {l: "create call", t: CALL_LOG_TYPE}, async log => { + create(type: "m.video" | "m.voice", log: ILogItem): Promise { + return log.wrap({l: "create call", t: CALL_LOG_TYPE}, async log => { if (this._state !== GroupCallState.Fledgling) { return; } From dfaaf6d23470b4d74707fba84ef5939be22213f0 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 17 Jan 2023 12:32:05 +0100 Subject: [PATCH 27/32] cleanup reply code a bit, have only 1 path to send message --- doc/impl-thoughts/RELATIONS.md | 2 +- src/domain/session/room/RoomViewModel.js | 7 ++++--- src/domain/session/room/timeline/tiles/BaseMessageTile.js | 4 ++-- src/matrix/room/timeline/entries/BaseEventEntry.js | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/doc/impl-thoughts/RELATIONS.md b/doc/impl-thoughts/RELATIONS.md index 5d91c28e..ac4e43ce 100644 --- a/doc/impl-thoughts/RELATIONS.md +++ b/doc/impl-thoughts/RELATIONS.md @@ -237,7 +237,7 @@ room.sendEvent(eventEntry.eventType, replacement); ## Replies ```js -const reply = eventEntry.reply({}); +const reply = eventEntry.createReplyContent({}); room.sendEvent("m.room.message", reply); ``` diff --git a/src/domain/session/room/RoomViewModel.js b/src/domain/session/room/RoomViewModel.js index 5a29bd85..8c551172 100644 --- a/src/domain/session/room/RoomViewModel.js +++ b/src/domain/session/room/RoomViewModel.js @@ -200,13 +200,14 @@ export class RoomViewModel extends ErrorReportViewModel { message = message.substr(4).trim(); msgtype = "m.emote"; } + let content; if (replyingTo) { log.set("replyingTo", replyingTo.eventId); - // TODO: reply should not send? it should just return the content for the reply and we send it ourselves - await replyingTo.reply(msgtype, message); + content = await replyingTo.createReplyContent(msgtype, message); } else { - await this._room.sendEvent("m.room.message", {msgtype, body: message}, undefined, log); + content = {msgtype, body: message}; } + await this._room.sendEvent("m.room.message", content, undefined, log); success = true; } log.set("success", success); diff --git a/src/domain/session/room/timeline/tiles/BaseMessageTile.js b/src/domain/session/room/timeline/tiles/BaseMessageTile.js index 5a28e1b3..44e0c5b0 100644 --- a/src/domain/session/room/timeline/tiles/BaseMessageTile.js +++ b/src/domain/session/room/timeline/tiles/BaseMessageTile.js @@ -146,8 +146,8 @@ export class BaseMessageTile extends SimpleTile { this._roomVM.startReply(this._entry); } - reply(msgtype, body, log = null) { - return this._room.sendEvent("m.room.message", this._entry.reply(msgtype, body), null, log); + createReplyContent(msgtype, body) { + return this._entry.createReplyContent(msgtype, body); } redact(reason, log) { diff --git a/src/matrix/room/timeline/entries/BaseEventEntry.js b/src/matrix/room/timeline/entries/BaseEventEntry.js index 44fdcaec..39698557 100644 --- a/src/matrix/room/timeline/entries/BaseEventEntry.js +++ b/src/matrix/room/timeline/entries/BaseEventEntry.js @@ -181,7 +181,7 @@ export class BaseEventEntry extends BaseEntry { return createAnnotation(this.id, key); } - reply(msgtype, body) { + createReplyContent(msgtype, body) { return createReplyContent(this, msgtype, body); } From bf9c868c8bc8f71cb9ed99301ccd0f580bcfb98d Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Tue, 17 Jan 2023 17:18:07 +0100 Subject: [PATCH 28/32] make it clearer that logAndCatch is probably what you want --- doc/error-handling.md | 6 ++++-- src/domain/ErrorReportViewModel.ts | 7 +++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/doc/error-handling.md b/doc/error-handling.md index 21c67ee5..ac2c4e59 100644 --- a/doc/error-handling.md +++ b/doc/error-handling.md @@ -2,12 +2,14 @@ Ideally, every error that is unexpected and can't be automatically recovered from without degrading the experience is shown in the UI. This is the task of the view model, and you can use `ErrorReportViewModel` for this purpose, a dedicated base view model class. It exposes a child view model, `ErrorViewModel`, when `reportError` is called which can be paired with `ErrorView` in the view to present an error message from which debug logs can also be sent. -Methods on classes from the `matrix` layer can often throw errors and those errors should be caught in the view model and reported with `reportError`. As a convenience method, there is also `logAndCatch` when inheriting from `ErrorReportViewModel` which combines logging and error reporting; it calls a callback within a log item and also a try catch that reports the error. +Methods on classes from the `matrix` layer can often throw errors and those errors should be caught in the view model and reported to the UI. When inheriting from `ErrorReportViewModel`, there is the low-level `reportError` method, but typically you'd use the convenience method `logAndCatch`. The latter makes it easy to get both error handlikng and logging right. You would typically use `logAndCatch` for every public method in the view model (e.g methods called from the view or from the parent view model). It calls a callback within a log item and also a try catch that reports the error. ## Sync errors & ErrorBoundary There are some errors that are thrown during background processes though, most notably the sync loop. These processes are not triggered by the view model directly, and hence there is not always a method call they can wrap in a try/catch. For this, there is the `ErrorBoundary` utility class. Since almost all aspects of the client can be updated through the sync loop, it is also not too helpful if there is only one try/catch around the whole sync and we stop sync if something goes wrong. -Instead, it's more helpful to split up the error handling into different scopes, where errors are stored and not rethrown when leaving the scope. One example is to have a scope per room. In this way, we can isolate an error occuring during sync to a specific room, and report it in the UI of that room. +Instead, it's more helpful to split up the error handling into different scopes, where errors are stored and not rethrown when leaving the scope. One example is to have a scope per room. In this way, we can isolate an error occuring during sync to a specific room, and report it in the UI of that room. This is typically where you would use `reportError` from `ErrorReportViewModel` rather than `logAndCatch`. You observe changes from your model in the view model (see docs on updates), and if the `error` property is set (by the `ErrorBoundary`), you call reportError with it. You can do this repeatedly without problems, if the same error is already reported, it's a No-Op. + +### `writeSync` and preventing data loss when dealing with errors. There is an extra complication though. The `writeSync` sync lifecycle step should not swallow any errors, or data loss can occur. This is because the whole `writeSync` lifecycle step writes all changes (for all rooms, the session, ...) for a sync response in one transaction (including the sync token), and aborts the transaction and stops sync if there is an error thrown during this step. So if there is an error in `writeSync` of a given room, it's fair to assume not all changes it was planning to write were passed to the transaction, as it got interrupted by the exception. Therefore, if we would swallow the error, data loss can occur as we'd not get another chance to write these changes to disk as we would have advanced the sync token. Therefore, code in the `writeSync` lifecycle step should be written defensively but always throw. diff --git a/src/domain/ErrorReportViewModel.ts b/src/domain/ErrorReportViewModel.ts index 68fe580c..da46b2df 100644 --- a/src/domain/ErrorReportViewModel.ts +++ b/src/domain/ErrorReportViewModel.ts @@ -32,6 +32,10 @@ export class ErrorReportViewModel extends ViewModel return this._errorViewModel; } + /** Typically you'd want to use `logAndCatch` when implementing a view model method. + * Use `reportError` when showing errors on your model that were set by + * background processes using `ErrorBoundary` or you have some other + * special low-level need to write your try/catch yourself. */ protected reportError(error: Error) { if (this._errorViewModel?.error === error) { return; @@ -47,6 +51,9 @@ export class ErrorReportViewModel extends ViewModel this.emitChange("errorViewModel"); } + /** Combines logging and error reporting in one method. + * Wrap the implementation of public view model methods + * with this to ensure errors are logged and reported.*/ protected logAndCatch(labelOrValues: LabelOrValues, callback: LogCallback, errorValue: T = undefined as unknown as T): T { try { let result = this.logger.run(labelOrValues, callback); From 3842f450dda28992767c62f382793fc8dc8f5d06 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 19 Jan 2023 11:30:23 +0100 Subject: [PATCH 29/32] ensure errors caught by boundary are logged in calls code --- src/matrix/calls/group/GroupCall.ts | 5 +++++ src/matrix/calls/group/Member.ts | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/src/matrix/calls/group/GroupCall.ts b/src/matrix/calls/group/GroupCall.ts index d700f2a0..25e201e5 100644 --- a/src/matrix/calls/group/GroupCall.ts +++ b/src/matrix/calls/group/GroupCall.ts @@ -94,6 +94,11 @@ export class GroupCall extends EventEmitter<{change: never}> { /** Set between calling join and leave. */ private joinedData?: JoinedData; private errorBoundary = new ErrorBoundary(err => { + if (this.joinedData) { + // in case the error happens in code that does not log, + // log it here to make sure it isn't swallowed + this.joinedData.logItem.log("error at boundary").catch(err); + } this.emitChange(); }); diff --git a/src/matrix/calls/group/Member.ts b/src/matrix/calls/group/Member.ts index 4ceaf46f..257628f5 100644 --- a/src/matrix/calls/group/Member.ts +++ b/src/matrix/calls/group/Member.ts @@ -97,6 +97,11 @@ export class Member { private expireTimeout?: Timeout; private errorBoundary = new ErrorBoundary(err => { this.options.emitUpdate(this, "error"); + if (this.connection) { + // in case the error happens in code that does not log, + // log it here to make sure it isn't swallowed + this.connection.logItem.log("error at boundary").catch(err); + } }); constructor( From e6b17cc74aeaa6a06e3799914fb4a6e7a0e79412 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 19 Jan 2023 11:30:52 +0100 Subject: [PATCH 30/32] fix callback type --- src/utils/ErrorBoundary.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/ErrorBoundary.ts b/src/utils/ErrorBoundary.ts index d13065f9..a4ac459c 100644 --- a/src/utils/ErrorBoundary.ts +++ b/src/utils/ErrorBoundary.ts @@ -19,7 +19,7 @@ export const ErrorValue = Symbol("ErrorBoundary:Error"); export class ErrorBoundary { private _error?: Error; - constructor(private readonly errorCallback: (Error) => void) {} + constructor(private readonly errorCallback: (err: Error) => void) {} /** * Executes callback() and then runs errorCallback() on error. @@ -79,4 +79,4 @@ export function tests() { assert.strictEqual(result, 0); } } -} \ No newline at end of file +} From 24088506785034a3164aac9f85811b561dcd8e19 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 19 Jan 2023 11:33:21 +0100 Subject: [PATCH 31/32] emit change before logging --- src/matrix/calls/group/GroupCall.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/matrix/calls/group/GroupCall.ts b/src/matrix/calls/group/GroupCall.ts index 25e201e5..f6436eeb 100644 --- a/src/matrix/calls/group/GroupCall.ts +++ b/src/matrix/calls/group/GroupCall.ts @@ -94,12 +94,12 @@ export class GroupCall extends EventEmitter<{change: never}> { /** Set between calling join and leave. */ private joinedData?: JoinedData; private errorBoundary = new ErrorBoundary(err => { + this.emitChange(); if (this.joinedData) { // in case the error happens in code that does not log, // log it here to make sure it isn't swallowed this.joinedData.logItem.log("error at boundary").catch(err); } - this.emitChange(); }); constructor( From daad19c0604305ed00e40cb06be42a987e3d56c8 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 19 Jan 2023 11:37:39 +0100 Subject: [PATCH 32/32] swallow errors in errorCallback in ErrorBoundary nothing should be able to make ErrorBoundary.try throw --- src/utils/ErrorBoundary.ts | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/utils/ErrorBoundary.ts b/src/utils/ErrorBoundary.ts index a4ac459c..a74da479 100644 --- a/src/utils/ErrorBoundary.ts +++ b/src/utils/ErrorBoundary.ts @@ -32,18 +32,26 @@ export class ErrorBoundary { if (result instanceof Promise) { result = result.catch(err => { this._error = err; - this.errorCallback(err); + this.reportError(err); return errorValue; }); } return result; } catch (err) { this._error = err; - this.errorCallback(err); + this.reportError(err); return errorValue; } } + private reportError(err: Error) { + try { + this.errorCallback(err); + } catch (err) { + console.error("error in ErrorBoundary callback", err); + } + } + get error(): Error | undefined { return this._error; } @@ -77,6 +85,15 @@ export function tests() { }, 0); assert(emitted); assert.strictEqual(result, 0); + }, + "exception in error callback is swallowed": async assert => { + let emitted = false; + const boundary = new ErrorBoundary(() => { throw new Error("bug in errorCallback"); }); + assert.doesNotThrow(() => { + boundary.try(() => { + throw new Error("fail!"); + }); + }); } } }