feature/23-voice-room/poc #93

Merged
hodlbod merged 68 commits from feature/23-voice-room/poc into dev 2026-03-16 20:38:06 +00:00
5 changed files with 97 additions and 106 deletions
Showing only changes of commit 6fcda91764 - Show all commits
+2 -4
View File
@@ -7,6 +7,7 @@
import {isMobileViewport} from "@lib/html"
import ProfileCircle from "@app/components/ProfileCircle.svelte"
import RoomName from "@app/components/RoomName.svelte"
import {errorMessage} from "@lib/util"
import {pushToast} from "@app/util/toast"
import {
deriveVoiceParticipants,
3
@@ -46,10 +47,7 @@
try {
await joinVoiceRoom(url, h, joinAbortController.signal)
} catch (e) {
if (e instanceof Error && e.message === "Join cancelled") return
if (e instanceof DOMException && e.name === "AbortError") return
const message = e instanceof Error ? e.message : String(e)
pushToast({theme: "error", message: `Failed to join voice room: ${message}`})
pushToast({theme: "error", message: `Failed to join voice room: ${errorMessage(e)}`})
} finally {
hodlbod marked this conversation as resolved Outdated
Outdated
Review

Why do the bots do this? I have seen this cause bugs more than once. Just do e.message || String(e). Or, better yet, actually design the error types and don't put anything that gets thrown in a message to the user. Just tell them "Failed to join voice room" and log it if it's not an error we know what to do with.

Why do the bots do this? I have seen this cause bugs more than once. Just do `e.message || String(e)`. Or, better yet, actually design the error types and don't put anything that gets thrown in a message to the user. Just tell them "Failed to join voice room" and log it if it's not an error we know what to do with.
Outdated
Review

I am cleaning this up but I want to fully understand you. Can you explain how this causes bugs? As a non-javascript guy this doesn't look pretty but makes sense to me because e could literally be any type and there is no way to write a catch block for a specific type, so it seems reasonably defensive to check the type before just grabbing the message property off of whatever might have been thrown. Or in the case where some library throws a String it seems reasonable to surface that.

I understand the current code risks showing raw error messages to the user (which doesn't bother me actually as long as it has a nice prefix like "Failed to join voice room"). Is there something worse that I am missing?

I am cleaning this up but I want to fully understand you. Can you explain how this causes bugs? As a non-javascript guy this doesn't look pretty but makes sense to me because `e` could literally be any type and there is no way to write a catch block for a specific type, so it seems reasonably defensive to check the type before just grabbing the `message` property off of whatever might have been thrown. Or in the case where some library throws a String it seems reasonable to surface that. I understand the current code risks showing raw error messages to the user (which doesn't bother me actually as long as it has a nice prefix like "Failed to join voice room"). Is there something worse that I am missing?
Outdated
Review

Idk maybe it is stupid to think that some library would throw some random object with a message property, but anything seems possible when node_modules are involved 😓

Idk maybe it is stupid to think that some library would throw some random object with a `message` property, but anything seems possible when `node_modules` are involved 😓
Outdated
Review

Oh interesting, e.message || String(e) actually gives a typescript error: 'e' is of type 'unknown'.. But AI found the errorMessage() function from @lib/util so I'll use that.

Oh interesting, `e.message || String(e)` actually gives a typescript error: `'e' is of type 'unknown'.`. But AI found the `errorMessage()` function from `@lib/util` so I'll use that.
Outdated
Review

The problem is when a pojo with a message gets thrown or rejected, which means the object gets cast to a string, which fails to show the message. This is the case when you probably actually want to show the message to the user, whereas showing an error's message is almost always confusing. Errors being completely untyped are a huge flaw in typescript, so I like to save throws for truly exceptional cases (https://effect.website/ does this more formally, and I think it's a good idea), in which case you want to log and recover if you can, and resolve/return everything else (even errors that are deferred to the caller). But I'm certainly not consistent about this.

errorMessage was introduced by an LLM too (via Tyson), I don't really believe in it either. I think we should just avoid propagating thrown errors to the user (I don't like the "Failed to do x: [technical jargon]" pattern. It helps to debug some, but not really in that many cases.

The problem is when a pojo with a message gets thrown or rejected, which means the object gets cast to a string, which fails to show the message. This is the case when you probably actually want to show the message to the user, whereas showing an error's message is almost always confusing. Errors being completely untyped are a huge flaw in typescript, so I like to save throws for truly exceptional cases (https://effect.website/ does this more formally, and I think it's a good idea), in which case you want to log and recover if you can, and resolve/return everything else (even errors that are deferred to the caller). But I'm certainly not consistent about this. `errorMessage` was introduced by an LLM too (via Tyson), I don't really believe in it either. I think we should just avoid propagating thrown errors to the user (I don't like the "Failed to do x: [technical jargon]" pattern. It helps to debug some, but not really in that many cases.
isJoining = false
joinAbortController = undefined
2
+3 -6
View File
@@ -1,5 +1,4 @@
<script lang="ts">
import {get} from "svelte/store"
import {displayRelayUrl} from "@welshman/util"
import Microphone from "@assets/icons/microphone.svg?dataurl"
import MicrophoneOff from "@assets/icons/microphone-off.svg?dataurl"
@@ -17,10 +16,8 @@
)
const spaceName = $derived($currentVoiceSession ? displayRelayUrl($currentVoiceSession.url) : "")
const handleDisconnect = () => leaveVoiceRoom()
const handleToggleMute = () => toggleMute()
const showRoomDetail = () => {
const session = get(currentVoiceSession)
const session = $currentVoiceSession
if (session) pushModal(RoomDetail, {url: session.url, h: session.h})
hodlbod marked this conversation as resolved Outdated
Outdated
Review

Don't do indirection like this, just call the actual function

Don't do indirection like this, just call the actual function
Outdated
Review

A stupid way to do this that I like would be ifLet($currentVoiceSession, ({url, h}) => pushModal(RoomDetail, {url, h})) but you don't have to do that

A stupid way to do this that I like would be `ifLet($currentVoiceSession, ({url, h}) => pushModal(RoomDetail, {url, h}))` but you don't have to do that
Outdated
Review

It is harder to read at first glance but ifLet brings warm feelings from better programming languages so it's done.

It is harder to read at first glance but `ifLet` brings warm feelings from better programming languages so it's done.
}
</script>
1
@@ -41,10 +38,10 @@
<div class="flex items-center gap-1">
<Button
class="btn btn-sm btn-square {$currentVoiceSession.muted ? 'btn-error' : 'btn-ghost'}"
onclick={handleToggleMute}>
onclick={toggleMute}>
<Icon icon={$currentVoiceSession.muted ? MicrophoneOff : Microphone} size={4} />
</Button>
<Button class="btn btn-sm btn-square btn-error" onclick={handleDisconnect}>
<Button class="btn btn-sm btn-square btn-error" onclick={leaveVoiceRoom}>
<Icon icon={PhoneRounded} size={4} />
</Button>
hodlbod marked this conversation as resolved Outdated
Outdated
Review

If we have room here, it might be good to label the buttons with mute/leave

If we have room here, it might be good to label the buttons with mute/leave
Outdated
Review

There is not a ton of room. How about a hover state?

There is not a ton of room. How about a hover state?
Outdated
Review

Yeah, that's a fine compromise.

Yeah, that's a fine compromise.
</div>
+5 -4
View File
@@ -103,6 +103,7 @@ import {
getListTags,
getPubkeyTagValues,
getRelayTagValues,
getTag,
getTagValues,
isRelayUrl,
normalizeRelayUrl,
4
@@ -668,7 +669,7 @@ export const deriveRoomsWithLivekit = (url: string) =>
derived(roomsById, $roomsById => {
const set = new Set<string>()
for (const room of $roomsById.values()) {
if (room.url === url && room.event?.tags?.some(t => t[0] === "livekit")) {
if (room.url === url && getTag("livekit", room.event?.tags ?? [])) {
set.add(room.h)
}
}
@@ -679,7 +680,7 @@ export const deriveRoomsNoText = (url: string) =>
derived(roomsById, $roomsById => {
const set = new Set<string>()
for (const room of $roomsById.values()) {
if (room.url === url && room.event?.tags?.some(t => t[0] === "no-text")) {
if (room.url === url && getTag("no-text", room.event?.tags ?? [])) {
set.add(room.h)
}
}
@@ -688,12 +689,12 @@ export const deriveRoomsNoText = (url: string) =>
export const roomHasLivekit = (url: string, h: string) => {
const room = getRoom(makeRoomId(url, h))
return room?.event?.tags?.some(t => t[0] === "livekit") ?? false
return !!getTag("livekit", room?.event?.tags ?? [])
}
export const roomIsNoText = (url: string, h: string) => {
const room = getRoom(makeRoomId(url, h))
return room?.event?.tags?.some(t => t[0] === "no-text") ?? false
return !!getTag("no-text", room?.event?.tags ?? [])
}
// User space/room lists
+57 -92
View File
@@ -3,12 +3,12 @@
* (ICE candidate gathering fails). Use Chrome or test from deployed HTTPS.
*/
import {DisconnectReason, Room, RoomEvent, Track} from "livekit-client"
import {getToken} from "nostr-tools/nip98"
import {derived, get, writable} from "svelte/store"
import {now} from "@welshman/lib"
import {makeEvent, getTagValue} from "@welshman/util"
import {makeEvent, makeHttpAuth, makeHttpAuthHeader, getTagValue} from "@welshman/util"
import {signer, publishThunk} from "@welshman/app"
import {getLivekitEndpoint} from "$lib/livekit"
import {AbortError, whenAborted, whenTimeout} from "$lib/util"
import {deriveEventsForUrl} from "@app/core/state"
import {pushToast} from "@app/util/toast"
@@ -28,7 +28,7 @@ export type VoiceSession = {
export const currentVoiceSession = writable<VoiceSession | undefined>(undefined)
export const speakingPubkeys = writable<Set<string>>(new Set())
export const speakingPubkeys = writable(new Set<string>())
const fetchLivekitToken = async (
url: string,
@@ -40,31 +40,16 @@ const fetchLivekitToken = async (
const $signer = signer.get()
if (!$signer) throw new Error("No signer available")
if (signal?.aborted) throw new Error("Join cancelled")
if (signal?.aborted) throw new DOMException("Aborted", "AbortError")
const authHeader = await getToken(
endpoint,
"GET",
template =>
$signer.sign(
makeEvent(template.kind, {
tags: template.tags,
content: template.content ?? "",
}),
),
true,
)
const template = await makeHttpAuth(endpoint, "GET")
const signedEvent = await $signer.sign(template)
const authHeader = makeHttpAuthHeader(signedEvent)
let response: Response
try {
response = await fetch(endpoint, {
headers: {Authorization: authHeader},
signal,
})
} catch (e) {
if (e instanceof DOMException && e.name === "AbortError") throw new Error("Join cancelled")
throw e
}
const response = await fetch(endpoint, {
hodlbod marked this conversation as resolved Outdated
Outdated
Review

[nit] simpler would be writable(new Set<string>())

[nit] simpler would be `writable(new Set<string>())`
headers: {Authorization: authHeader},
signal,
})
if (!response.ok) {
const text = await response.text()
3
@@ -119,6 +104,36 @@ const stopPresenceHeartbeat = () => {
}
}
const onRoomDisconnected = (reason?: DisconnectReason) => {
speakingPubkeys.set(new Set())
currentVoiceSession.set(undefined)
stopPresenceHeartbeat()
if (reason !== undefined && reason !== DisconnectReason.CLIENT_INITIATED) {
const message =
reason === DisconnectReason.JOIN_FAILURE
? "Could not connect to voice room. Please try again."
: "Voice connection lost."
pushToast({theme: "error", message})
}
}
const onTrackSubscribed = (track: Track) => {
if (track.kind === Track.Kind.Audio) {
const element = track.attach()
element.style.display = "none"
document.body.appendChild(element)
element.play().catch(() => {})
}
}
const onTrackUnsubscribed = (track: Track) => {
track.detach().forEach(el => el.remove())
}
const onActiveSpeakersChanged = (participants: {identity: string}[]) => {
speakingPubkeys.set(new Set(participants.map(p => p.identity)))
}
export const joinVoiceRoom = async (
url: string,
h: string,
@@ -126,83 +141,33 @@ export const joinVoiceRoom = async (
): Promise<void> => {
const session = get(currentVoiceSession)
if (session) {
if (session.url === url && session.h === h) return
await leaveVoiceRoom()
}
if (session) await leaveVoiceRoom()
const {server_url, participant_token} = await fetchLivekitToken(url, h, signal)
if (signal?.aborted) throw new Error("Join cancelled")
if (signal?.aborted) return
const room = new Room({
adaptiveStream: true,
dynacast: true,
const room = new Room({adaptiveStream: true, dynacast: true})
room.on(RoomEvent.Disconnected, onRoomDisconnected)
room.on(RoomEvent.TrackSubscribed, onTrackSubscribed)
room.on(RoomEvent.TrackUnsubscribed, onTrackUnsubscribed)
room.on(RoomEvent.ActiveSpeakersChanged, onActiveSpeakersChanged)
const connect = room.connect(server_url, participant_token, {maxRetries: 0})
const timeout = whenTimeout(5_000, {
message: "Connection timed out. Please check your network and try again.",
})
room.on(RoomEvent.Disconnected, (reason?: DisconnectReason) => {
speakingPubkeys.set(new Set())
currentVoiceSession.set(undefined)
stopPresenceHeartbeat()
if (reason !== undefined && reason !== DisconnectReason.CLIENT_INITIATED) {
const message =
reason === DisconnectReason.JOIN_FAILURE
? "Could not connect to voice room. Please try again."
: "Voice connection lost."
pushToast({theme: "error", message})
}
})
room.on(RoomEvent.TrackSubscribed, (track, _publication, _participant) => {
if (track.kind === Track.Kind.Audio) {
const element = track.attach()
element.style.display = "none"
document.body.appendChild(element)
element.play().catch(() => {})
}
})
room.on(RoomEvent.TrackUnsubscribed, track => {
track.detach().forEach(el => el.remove())
})
room.on(RoomEvent.ActiveSpeakersChanged, participants => {
speakingPubkeys.set(new Set(participants.map(p => p.identity)))
})
const onAbort = () => {
room.disconnect()
}
if (signal) {
if (signal.aborted) {
room.disconnect()
throw new Error("Join cancelled")
}
signal.addEventListener("abort", onAbort, {once: true})
}
const CONNECT_TIMEOUT_MS = 5_000
const abort = whenAborted(signal)
try {
await Promise.race([
room.connect(server_url, participant_token, {maxRetries: 0}),
new Promise<never>((_, reject) =>
setTimeout(
() => reject(new Error("Connection timed out. Please check your network and try again.")),
CONNECT_TIMEOUT_MS,
),
),
])
await Promise.race([connect, timeout, abort])
} catch (e) {
room.disconnect()
if (signal?.aborted) {
throw new Error("Join cancelled")
}
if (e instanceof AbortError) return
throw e
} finally {
signal?.removeEventListener("abort", onAbort)
}
if (signal?.aborted) throw new Error("Join cancelled")
await room.localParticipant.setMicrophoneEnabled(true)
currentVoiceSession.set({url, h, room, muted: false})
+30
View File
@@ -19,6 +19,36 @@ export const ucFirst = (s: string) => s.slice(0, 1).toUpperCase() + s.slice(1)
export const errorMessage = (err: unknown) => String(err).replace(/^.*Error: /, "")
export class AbortError extends Error {
constructor() {
super("Aborted")
this.name = "AbortError"
}
}
export class TimeoutError extends Error {
constructor(message = "Timed out") {
super(message)
this.name = "TimeoutError"
}
}
/** Returns a promise that rejects with AbortError when signal aborts. Use with Promise.race. */
export const whenAborted = (signal?: AbortSignal) => {
if (!signal) return new Promise<never>(() => {})
return new Promise<never>((_, reject) => {
const onAborted = () => reject(new AbortError())
if (signal.aborted) onAborted()
else signal.addEventListener("abort", onAborted, {once: true})
})
}
/** Returns a promise that rejects with TimeoutError after ms. Use with Promise.race. */
export const whenTimeout = (ms: number, opts: {message?: string} = {}) => {
return new Promise<never>((_, reject) => setTimeout(() => reject(new TimeoutError()), ms))
}
export const buildUrl = (base: string | URL, ...pathname: string[]) => {
const url = new URL(base)