From bc85cd49846da4f1f3802cfb7aa2f9ffea859c76 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 26 Jun 2026 18:15:51 -0400 Subject: [PATCH] fix(calls,matrix): address review findings from agent code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CallEmbed watchdog now SELF-HEALS: a genuine ready/joined signal arriving after the 25s timeout clears the error and notifies subscribers with undefined, so a slow-but-successful EC load no longer strands the user on the recovery screen over a live call. Listener dispatch wrapped in try/catch. - ringtones: synth notes route through a per-session master gain; stop() ramps it to 0 so the ring is silenced instantly on answer instead of letting the last scheduled phrase ring out over call audio. - IncomingCallBanner: ping fires exactly once per incoming call (guarded by refEventId) instead of re-pinging when ringtone settings change mid-banner. - focusCameraParticipant: try multiple tile selectors (EC labels vary by version), defer the tile click past EC's async spotlight layout switch (rAF x2), and dev-warn when no tile matches so testers get signal. - uploadContent: a cancelled upload (mx.cancelUpload -> AbortError) is no longer treated as retryable — previously the retry loop could resurrect an upload the user just cancelled. Also retry on 408. - addRoomIdToMDirect/removeRoomIdFromMDirect: guard against a corrupt m.direct whose values aren't arrays. Co-Authored-By: Claude Opus 4.8 --- src/app/components/CallEmbedProvider.tsx | 9 ++++- src/app/plugins/call/CallControl.ts | 50 ++++++++++++++++++------ src/app/plugins/call/CallEmbed.ts | 45 +++++++++++++++++---- src/app/utils/matrix.ts | 14 +++++-- src/app/utils/ringtones.ts | 44 +++++++++++++++++---- 5 files changed, 131 insertions(+), 31 deletions(-) diff --git a/src/app/components/CallEmbedProvider.tsx b/src/app/components/CallEmbedProvider.tsx index b7cd42367..b40d5ad14 100644 --- a/src/app/components/CallEmbedProvider.tsx +++ b/src/app/components/CallEmbedProvider.tsx @@ -289,11 +289,16 @@ function IncomingCallBanner({ dm, info, onIgnore, onAnswer, onReject }: Incoming ); // Single soft ping (non-looping) on arrival, respecting the chosen ringtone - // + volume. We intentionally do NOT loop here — the user is mid-call. + // + volume. We intentionally do NOT loop here — the user is mid-call — and we + // ping exactly once per incoming call, not again if the user happens to tweak + // ringtone settings while the banner is showing. + const pingedRef = useRef(undefined); useEffect(() => { if (info.notificationType !== 'ring') return; + if (pingedRef.current === info.refEventId) return; + pingedRef.current = info.refEventId; previewRingtone(ringtoneId, Math.max(0, Math.min(1, ringtoneVolume / 100))); - }, [info.notificationType, ringtoneId, ringtoneVolume]); + }, [info.notificationType, info.refEventId, ringtoneId, ringtoneVolume]); useEffect(() => { const remaining = info.senderTs + info.lifetime - Date.now(); diff --git a/src/app/plugins/call/CallControl.ts b/src/app/plugins/call/CallControl.ts index b87a620fa..1e34e8a18 100644 --- a/src/app/plugins/call/CallControl.ts +++ b/src/app/plugins/call/CallControl.ts @@ -356,20 +356,48 @@ export class CallControl extends EventEmitter implements CallControlState { const doc = this.document; if (!doc) return; - // Find the mute icon / aria-label element that identifies this participant - const userEl = doc.querySelector(`[aria-label="${CSS.escape(userId)}"]`); - // Walk up to the nearest video tile container - const tile = - userEl?.closest('[data-testid="videoTile"]') ?? - userEl?.closest('[data-video-fit]'); + // EC labels participant tiles inconsistently across versions — the user's + // matrix id may be the full aria-label, a substring of it, or carried on a + // data attribute (and sometimes the visible label is the display name, not + // the id at all). Try several strategies before giving up, then walk up to + // the enclosing video tile. + const findTile = (): HTMLElement | undefined => { + const escaped = CSS.escape(userId); + const el = + doc.querySelector(`[aria-label="${escaped}"]`) ?? + doc.querySelector(`[data-testid="videoTile"][aria-label*="${escaped}"]`) ?? + doc.querySelector(`[aria-label*="${escaped}"]`) ?? + doc.querySelector(`[data-member-id="${escaped}"]`) ?? + doc.querySelector(`[data-id="${escaped}"]`) ?? + undefined; + return ( + el?.closest('[data-testid="videoTile"]') ?? + el?.closest('[data-video-fit]') ?? + el ?? + undefined + ); + }; - if (!this.spotlight) { - this.spotlightButton?.click(); + const applyFocus = () => { + const tile = findTile(); + if (tile) { + tile.click(); + } else if (import.meta.env.DEV) { + console.warn(`[CallControl] focusCameraParticipant: no tile matched ${userId}`); + } + }; + + if (this.spotlight) { + // Already in spotlight — pin immediately. + applyFocus(); + return; } - if (tile) { - tile.click(); - } + // Switching to spotlight re-renders EC's layout asynchronously; clicking the + // tile in the same tick would land in the old (grid) DOM. Toggle spotlight, + // then click on a later frame once the spotlight tiles have mounted. + this.spotlightButton?.click(); + requestAnimationFrame(() => requestAnimationFrame(applyFocus)); } public dispose() { diff --git a/src/app/plugins/call/CallEmbed.ts b/src/app/plugins/call/CallEmbed.ts index 9f1387107..8aea8e4d8 100644 --- a/src/app/plugins/call/CallEmbed.ts +++ b/src/app/plugins/call/CallEmbed.ts @@ -70,7 +70,9 @@ export class CallEmbed { private loadError?: CallLoadErrorReason; - private readonly loadErrorListeners = new Set<(reason: CallLoadErrorReason) => void>(); + private readonly loadErrorListeners = new Set< + (reason: CallLoadErrorReason | undefined) => void + >(); // Arrow-function class fields so dispose() passes the exact same reference to mx.off() private readonly boundOnEvent = (ev: MatrixEvent) => this.onEvent(ev); @@ -375,17 +377,44 @@ export class CallEmbed { } } + private notifyLoadListeners(reason: CallLoadErrorReason | undefined): void { + this.loadErrorListeners.forEach((cb) => { + try { + cb(reason); + } catch { + // a misbehaving subscriber must not block the others + } + }); + } + /** - * Marks the load lifecycle as settled. Called on success (no reason) or on - * failure (reason set). Idempotent so the first signal wins. + * Marks the load lifecycle as settled. + * + * - Failure (reason set): the FIRST failure wins; a later success can still + * heal it (below). Once we've genuinely succeeded, later spurious failures + * are ignored. + * - Success (no reason): always clears the watchdog. Crucially, if we had + * previously settled as a failure (e.g. the 25s watchdog fired on a slow + * network but EC then finished loading), we self-heal: clear the error and + * notify subscribers with `undefined` so the recovery UI dismisses itself + * instead of stranding the user on an error screen over a live call. */ private settleLoad(reason?: CallLoadErrorReason): void { - if (this.loadSettled) return; - this.loadSettled = true; - this.clearLoadWatchdog(); if (reason) { + if (this.loadSettled) return; + this.loadSettled = true; + this.clearLoadWatchdog(); this.loadError = reason; - this.loadErrorListeners.forEach((cb) => cb(reason)); + this.notifyLoadListeners(reason); + return; + } + + this.clearLoadWatchdog(); + const wasFailed = this.loadError !== undefined; + this.loadSettled = true; + this.loadError = undefined; + if (wasFailed) { + this.notifyLoadListeners(undefined); } } @@ -402,7 +431,7 @@ export class CallEmbed { * immediately so late subscribers still see the error. * @returns an unsubscribe function. */ - public onLoadError(callback: (reason: CallLoadErrorReason) => void): () => void { + public onLoadError(callback: (reason: CallLoadErrorReason | undefined) => void): () => void { this.loadErrorListeners.add(callback); if (this.loadError) callback(this.loadError); return () => { diff --git a/src/app/utils/matrix.ts b/src/app/utils/matrix.ts index 13d18b5a3..7661b7527 100644 --- a/src/app/utils/matrix.ts +++ b/src/app/utils/matrix.ts @@ -169,12 +169,17 @@ const matrixErrorFromUnknown = (e: unknown): MatrixError => { // HTTP statuses that should not be retried — client errors are deterministic // (e.g. 413 payload too large, 400 bad request, 401/403 auth) and won't succeed on retry. const isRetryableUploadError = (e: unknown): boolean => { + // A user-cancelled / aborted upload must never be retried. matrix-js-sdk's + // mx.cancelUpload() rejects the upload with a DOMException named "AbortError"; + // without this guard the retry loop would resurrect an upload the user just + // cancelled. + if ((e as { name?: unknown } | null | undefined)?.name === 'AbortError') return false; if (e instanceof MatrixError) { const status = e.httpStatus; // No status => network/transport failure (transient): retry. if (typeof status !== 'number') return true; - // Retry on rate-limiting and server-side (5xx) errors only. - return status === 429 || status >= 500; + // Retry on request-timeout, rate-limiting and server-side (5xx) errors only. + return status === 408 || status === 429 || status >= 500; } // Non-Matrix errors are typically network/transport failures: retry. return true; @@ -307,6 +312,8 @@ export const addRoomIdToMDirect = async ( // (it can only be a DM room for one person) Object.keys(userIdToRoomIds).forEach((targetUserId) => { const roomIds = userIdToRoomIds[targetUserId]; + // Guard against a corrupt m.direct where a value isn't an array. + if (!Array.isArray(roomIds)) return; if (targetUserId !== userId) { const indexOfRoomId = roomIds.indexOf(roomId); @@ -316,7 +323,7 @@ export const addRoomIdToMDirect = async ( } }); - const roomIds = userIdToRoomIds[userId] || []; + const roomIds = Array.isArray(userIdToRoomIds[userId]) ? userIdToRoomIds[userId] : []; if (roomIds.indexOf(roomId) === -1) { roomIds.push(roomId); } @@ -334,6 +341,7 @@ export const removeRoomIdFromMDirect = async (mx: MatrixClient, roomId: string): Object.keys(userIdToRoomIds).forEach((targetUserId) => { const roomIds = userIdToRoomIds[targetUserId]; + if (!Array.isArray(roomIds)) return; const indexOfRoomId = roomIds.indexOf(roomId); if (indexOfRoomId > -1) { roomIds.splice(indexOfRoomId, 1); diff --git a/src/app/utils/ringtones.ts b/src/app/utils/ringtones.ts index 31d4c4e76..d7ee22cff 100644 --- a/src/app/utils/ringtones.ts +++ b/src/app/utils/ringtones.ts @@ -78,7 +78,7 @@ const PHRASES: Record< }, }; -const playPhrase = (style: SynthStyle, volume: number): void => { +const playPhrase = (style: SynthStyle, volume: number, destination: AudioNode): void => { const ctx = getCtx(); if (!ctx) return; const { type, gain: peak, notes } = PHRASES[style]; @@ -96,7 +96,7 @@ const playPhrase = (style: SynthStyle, volume: number): void => { gain.gain.linearRampToValueAtTime(scaledPeak, start + 0.015); gain.gain.exponentialRampToValueAtTime(0.0001, start + dur); osc.connect(gain); - gain.connect(ctx.destination); + gain.connect(destination); osc.start(start); osc.stop(start + dur + 0.02); }); @@ -121,11 +121,41 @@ const startClassic = (volume: number, loop: boolean): (() => void) => { }; const startSynth = (style: SynthStyle, volume: number, loop: boolean): (() => void) => { - playPhrase(style, volume); - if (!loop) return () => undefined; - const period = PHRASES[style].period * 1000; - const id = window.setInterval(() => playPhrase(style, volume), period); - return () => window.clearInterval(id); + const ctx = getCtx(); + if (!ctx) return () => undefined; + // All notes route through a per-session master gain so stop() can silence + // everything instantly — including notes already scheduled slightly in the + // future — instead of letting the last phrase ring out after the user answers. + const master = ctx.createGain(); + master.gain.value = 1; + master.connect(ctx.destination); + + playPhrase(style, volume, master); + const id = loop + ? window.setInterval(() => playPhrase(style, volume, master), PHRASES[style].period * 1000) + : 0; + + let stopped = false; + return () => { + if (stopped) return; + stopped = true; + if (id) window.clearInterval(id); + try { + const now = ctx.currentTime; + master.gain.cancelScheduledValues(now); + master.gain.setValueAtTime(master.gain.value, now); + master.gain.linearRampToValueAtTime(0, now + 0.03); + } catch { + /* context may be closed */ + } + window.setTimeout(() => { + try { + master.disconnect(); + } catch { + /* already disconnected */ + } + }, 100); + }; }; /**