feature/23-voice-room/poc #93
@@ -6,7 +6,7 @@
|
||||
import ImageIcon from "@lib/components/ImageIcon.svelte"
|
||||
|
||||
type Props = {
|
||||
pubkey: string
|
||||
pubkey?: string
|
||||
class?: string
|
||||
size?: number
|
||||
url?: string
|
||||
@@ -14,11 +14,11 @@
|
||||
|
||||
const {pubkey, url, size = 7, ...props}: Props = $props()
|
||||
|
||||
const profile = deriveProfile(pubkey, removeUndefined([url]))
|
||||
const profile = pubkey ? deriveProfile(pubkey, removeUndefined([url])) : undefined
|
||||
</script>
|
||||
|
||||
<ImageIcon
|
||||
{size}
|
||||
|
|
||||
alt=""
|
||||
class={cx(props.class, "rounded-full")}
|
||||
src={$profile?.picture || UserRounded} />
|
||||
src={(profile ? $profile?.picture : undefined) || UserRounded} />
|
||||
|
||||
@@ -10,7 +10,9 @@
|
||||
import {
|
||||
deriveVoiceParticipants,
|
||||
joinVoiceRoom,
|
||||
currentVoiceSession,
|
||||
cancelJoinVoiceRoom,
|
||||
currentVoiceRoom,
|
||||
voiceState,
|
||||
isParticipantSpeaking,
|
||||
participantKey,
|
||||
type VoiceParticipant,
|
||||
@@ -25,25 +27,26 @@
|
||||
const {url, h, replaceState = false}: Props = $props()
|
||||
|
||||
const participants = deriveVoiceParticipants(url, h)
|
||||
const isActive = $derived($currentVoiceSession?.url === url && $currentVoiceSession?.h === h)
|
||||
let joinAbortController = $state<AbortController | undefined>(undefined)
|
||||
const isActive = $derived(
|
||||
$voiceState === "connected" && $currentVoiceRoom?.url === url && $currentVoiceRoom?.h === h,
|
||||
)
|
||||
const isJoining = $derived(
|
||||
$voiceState === "joining" && $currentVoiceRoom?.url === url && $currentVoiceRoom?.h === h,
|
||||
)
|
||||
|
hodlbod marked this conversation as resolved
Outdated
hodlbod
commented
Instead of this, we should just not render voice rooms on mobile Instead of this, we should just not render voice rooms on mobile
mplorentz
commented
I tried that, but realized that you could then join a call on desktop web, resize your browser to be small and then lose the UI to leave the call. As I type it out I realize that's too edgy of a case 😂 I will just hide it. I tried that, but realized that you could then join a call on desktop web, resize your browser to be small and then lose the UI to leave the call. As I type it out I realize that's too edgy of a case 😂 I will just hide it.
|
||||
|
||||
const handleClick = async () => {
|
||||
if (isActive) {
|
||||
if (isActive) return
|
||||
|
||||
if (isJoining) {
|
||||
cancelJoinVoiceRoom()
|
||||
return
|
||||
}
|
||||
if (joinAbortController) {
|
||||
joinAbortController.abort()
|
||||
return
|
||||
}
|
||||
joinAbortController = new AbortController()
|
||||
|
||||
try {
|
||||
|
hodlbod marked this conversation as resolved
Outdated
hodlbod
commented
[nit] in situations like this, I like to overload the controller as the isJoining boolean and just check whether joinAbortController is undefined. One fewer variable to keep track of [nit] in situations like this, I like to overload the controller as the isJoining boolean and just check whether joinAbortController is undefined. One fewer variable to keep track of
|
||||
await joinVoiceRoom(url, h, joinAbortController.signal)
|
||||
await joinVoiceRoom(url, h)
|
||||
} catch (e) {
|
||||
console.error("Failed to join voice room", e)
|
||||
pushToast({theme: "error", message: "Failed to join voice room"})
|
||||
} finally {
|
||||
joinAbortController = undefined
|
||||
}
|
||||
}
|
||||
|
hodlbod marked this conversation as resolved
Outdated
hodlbod
commented
Why do the bots do this? I have seen this cause bugs more than once. Just do 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.
mplorentz
commented
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 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?
mplorentz
commented
Idk maybe it is stupid to think that some library would throw some random object with a 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 😓
mplorentz
commented
Oh interesting, 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.
hodlbod
commented
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.
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.
|
||||
|
||||
@@ -61,7 +64,7 @@
|
||||
class={cx("!items-start", isActive && "!bg-base-100 !text-base-content")}>
|
||||
<div class="flex w-full min-w-0 flex-col gap-2">
|
||||
<div class="flex gap-2 items-center">
|
||||
{#if joinAbortController}
|
||||
{#if isJoining}
|
||||
<span class="loading loading-spinner loading-sm"></span>
|
||||
{:else}
|
||||
<RoomImage {url} {h} size={4} />
|
||||
|
||||
@@ -4,43 +4,76 @@
|
||||
import Microphone from "@assets/icons/microphone.svg?dataurl"
|
||||
import MicrophoneOff from "@assets/icons/microphone-off.svg?dataurl"
|
||||
import PhoneRounded from "@assets/icons/phone-rounded.svg?dataurl"
|
||||
import PhoneCallingRounded from "@assets/icons/phone-calling-rounded.svg?dataurl"
|
||||
import CloseCircle from "@assets/icons/close-circle.svg?dataurl"
|
||||
import Icon from "@lib/components/Icon.svelte"
|
||||
import Button from "@lib/components/Button.svelte"
|
||||
import {displayRoom} from "@app/core/state"
|
||||
import {currentVoiceSession, leaveVoiceRoom, toggleMute} from "@app/voice"
|
||||
import {
|
||||
currentVoiceSession,
|
||||
currentVoiceRoom,
|
||||
voiceState,
|
||||
leaveVoiceRoom,
|
||||
toggleMute,
|
||||
rejoinVoiceRoom,
|
||||
cancelJoinVoiceRoom,
|
||||
} from "@app/voice"
|
||||
|
||||
|
hodlbod marked this conversation as resolved
Outdated
hodlbod
commented
Don't do indirection like this, just call the actual function Don't do indirection like this, just call the actual function
hodlbod
commented
A stupid way to do this that I like would be 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
mplorentz
commented
It is harder to read at first glance but It is harder to read at first glance but `ifLet` brings warm feelings from better programming languages so it's done.
|
||||
const roomName = $derived(
|
||||
$currentVoiceSession ? displayRoom($currentVoiceSession.url, $currentVoiceSession.h) : "",
|
||||
$currentVoiceRoom ? displayRoom($currentVoiceRoom.url, $currentVoiceRoom.h) : "",
|
||||
)
|
||||
|
hodlbod marked this conversation as resolved
Outdated
hodlbod
commented
$currentVoiceSession is better than $currentVoiceSession is better than `get`
|
||||
const spaceName = $derived($currentVoiceSession ? displayRelayUrl($currentVoiceSession.url) : "")
|
||||
const spaceName = $derived($currentVoiceRoom ? displayRelayUrl($currentVoiceRoom.url) : "")
|
||||
</script>
|
||||
|
||||
{#if $currentVoiceSession}
|
||||
{#if $currentVoiceRoom}
|
||||
<div
|
||||
in:fly={{y: 60, duration: 350}}
|
||||
out:fly={{y: 60, duration: 250}}
|
||||
class="flex flex-col gap-2 rounded-box bg-base-100 p-3">
|
||||
<div class="flex flex-col gap-0.5">
|
||||
<span class="text-sm font-semibold text-success">Voice Connected</span>
|
||||
{#if $voiceState === "joining"}
|
||||
<span class="text-sm font-semibold text-warning">Joining...</span>
|
||||
{:else if $voiceState === "connected"}
|
||||
<span class="text-sm font-semibold text-success">Voice Connected</span>
|
||||
{:else}
|
||||
<span class="text-sm font-semibold text-neutral-content">Disconnected</span>
|
||||
{/if}
|
||||
<span class="ellipsize text-xs opacity-70">
|
||||
{roomName} / {spaceName}
|
||||
</span>
|
||||
</div>
|
||||
<div class="flex items-center gap-1">
|
||||
<Button
|
||||
data-tip={$currentVoiceSession.muted ? "Unmute" : "Mute"}
|
||||
class="center tooltip tooltip-top btn btn-sm btn-square {$currentVoiceSession.muted
|
||||
? 'btn-error'
|
||||
: 'btn-ghost'}"
|
||||
onclick={toggleMute}>
|
||||
<Icon icon={$currentVoiceSession.muted ? MicrophoneOff : Microphone} size={4} />
|
||||
</Button>
|
||||
<Button
|
||||
data-tip="Leave room"
|
||||
class="center tooltip tooltip-top btn btn-sm btn-square btn-error"
|
||||
onclick={leaveVoiceRoom}>
|
||||
<Icon icon={PhoneRounded} size={4} />
|
||||
</Button>
|
||||
{#if $voiceState === "joining"}
|
||||
|
hodlbod marked this conversation as resolved
Outdated
hodlbod
commented
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
mplorentz
commented
There is not a ton of room. How about a hover state? There is not a ton of room. How about a hover state?
hodlbod
commented
Yeah, that's a fine compromise. Yeah, that's a fine compromise.
|
||||
<span class="loading loading-spinner loading-sm"></span>
|
||||
<Button
|
||||
data-tip="Cancel"
|
||||
class="center tooltip tooltip-top btn btn-sm btn-square btn-ghost"
|
||||
onclick={cancelJoinVoiceRoom}>
|
||||
<Icon icon={CloseCircle} size={4} />
|
||||
</Button>
|
||||
{:else if $voiceState === "connected" && $currentVoiceSession}
|
||||
<Button
|
||||
data-tip={$currentVoiceSession.muted ? "Unmute" : "Mute"}
|
||||
class="center tooltip tooltip-top btn btn-sm btn-square {$currentVoiceSession.muted
|
||||
? 'btn-error'
|
||||
: 'btn-ghost'}"
|
||||
onclick={toggleMute}>
|
||||
<Icon icon={$currentVoiceSession.muted ? MicrophoneOff : Microphone} size={4} />
|
||||
</Button>
|
||||
<Button
|
||||
data-tip="Leave room"
|
||||
class="center tooltip tooltip-top btn btn-sm btn-square btn-error"
|
||||
onclick={leaveVoiceRoom}>
|
||||
<Icon icon={PhoneRounded} size={4} />
|
||||
</Button>
|
||||
{:else}
|
||||
<Button
|
||||
data-tip="Join Voice"
|
||||
class="center tooltip tooltip-top btn btn-sm btn-square btn-success"
|
||||
onclick={rejoinVoiceRoom}>
|
||||
<Icon icon={PhoneCallingRounded} size={4} />
|
||||
</Button>
|
||||
{/if}
|
||||
</div>
|
||||
</div>
|
||||
{/if}
|
||||
|
||||
@@ -28,8 +28,14 @@ export type Pubkey = string
|
||||
|
||||
export type VoiceParticipant = {pubkey?: Pubkey; identity: string}
|
||||
|
||||
export type VoiceState = "joining" | "connected" | "disconnected"
|
||||
|
||||
export const currentVoiceSession = writable<VoiceSession | undefined>(undefined)
|
||||
|
||||
export const voiceState = writable<VoiceState>("disconnected")
|
||||
|
||||
export const currentVoiceRoom = writable<{url: string; h: string} | undefined>(undefined)
|
||||
|
||||
export const participantPubkeyMap = writable<Map<string, Pubkey>>(new Map())
|
||||
|
||||
const addParticipant = (identity: string) => {
|
||||
@@ -48,8 +54,6 @@ const deleteParticipant = (identity: string) => {
|
||||
})
|
||||
}
|
||||
|
||||
const currentVoiceRoom = derived(currentVoiceSession, s => (s ? {url: s.url, h: s.h} : undefined))
|
||||
|
||||
export const pubkeyFromLiveKitIdentity = (identity: string): string | undefined =>
|
||||
/^[a-f0-9]{64}$/.test(identity.slice(0, 64)) ? identity.slice(0, 64) : undefined
|
||||
|
||||
@@ -133,6 +137,7 @@ const onRoomDisconnected = (reason?: DisconnectReason) => {
|
||||
participantPubkeyMap.set(new Map())
|
||||
currentVoiceSession.set(undefined)
|
||||
if (reason !== undefined && reason !== DisconnectReason.CLIENT_INITIATED) {
|
||||
voiceState.set("disconnected")
|
||||
const message =
|
||||
reason === DisconnectReason.JOIN_FAILURE
|
||||
? "Could not connect to voice room. Please try again."
|
||||
@@ -172,59 +177,77 @@ const onParticipantDisconnected = (participant: {identity: string}) => {
|
||||
deleteParticipant(participant.identity)
|
||||
}
|
||||
|
||||
export const joinVoiceRoom = async (
|
||||
url: string,
|
||||
h: string,
|
||||
signal?: AbortSignal,
|
||||
): Promise<void> => {
|
||||
const session = get(currentVoiceSession)
|
||||
let joinAbortController: AbortController | undefined
|
||||
|
||||
export const cancelJoinVoiceRoom = () => {
|
||||
joinAbortController?.abort()
|
||||
}
|
||||
|
||||
export const joinVoiceRoom = async (url: string, h: string): Promise<void> => {
|
||||
cancelJoinVoiceRoom()
|
||||
|
||||
const session = get(currentVoiceSession)
|
||||
if (session) await leaveVoiceRoom()
|
||||
|
||||
const {server_url, participant_token} = await fetchLivekitToken(url, h, signal)
|
||||
currentVoiceRoom.set({url, h})
|
||||
voiceState.set("joining")
|
||||
|
||||
if (signal?.aborted) return
|
||||
|
||||
const room = new Room({adaptiveStream: true, dynacast: true})
|
||||
|
||||
room.on(RoomEvent.Disconnected, onRoomDisconnected)
|
||||
room.on(RoomEvent.ParticipantConnected, onParticipantConnected)
|
||||
room.on(RoomEvent.ParticipantDisconnected, onParticipantDisconnected)
|
||||
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.",
|
||||
})
|
||||
const abort = whenAborted(signal)
|
||||
const controller = new AbortController()
|
||||
joinAbortController = controller
|
||||
const signal = controller.signal
|
||||
const isActive = () => joinAbortController === controller
|
||||
|
hodlbod marked this conversation as resolved
Outdated
hodlbod
commented
This will never happen because there are no awaits between this and the previous check. The if (signal) is redundant too, we should just require the caller to provide a signal. This will never happen because there are no awaits between this and the previous check.
The if (signal) is redundant too, we should just require the caller to provide a signal.
|
||||
|
||||
try {
|
||||
await Promise.race([connect, timeout, abort])
|
||||
const {server_url, participant_token} = await fetchLivekitToken(url, h, signal)
|
||||
|
||||
if (signal.aborted) throw new AbortError()
|
||||
|
||||
const room = new Room({adaptiveStream: true, dynacast: true})
|
||||
|
||||
room.on(RoomEvent.Disconnected, onRoomDisconnected)
|
||||
room.on(RoomEvent.ParticipantConnected, onParticipantConnected)
|
||||
room.on(RoomEvent.ParticipantDisconnected, onParticipantDisconnected)
|
||||
room.on(RoomEvent.TrackSubscribed, onTrackSubscribed)
|
||||
room.on(RoomEvent.TrackUnsubscribed, onTrackUnsubscribed)
|
||||
room.on(RoomEvent.ActiveSpeakersChanged, onActiveSpeakersChanged)
|
||||
|
||||
try {
|
||||
await Promise.race([
|
||||
room.connect(server_url, participant_token, {maxRetries: 0}),
|
||||
whenTimeout(5_000, {
|
||||
message: "Connection timed out. Please check your network and try again.",
|
||||
|
hodlbod marked this conversation as resolved
Outdated
hodlbod
commented
too much of this noise too much of this noise
|
||||
}),
|
||||
whenAborted(signal),
|
||||
])
|
||||
} catch (e) {
|
||||
room.disconnect()
|
||||
|
hodlbod marked this conversation as resolved
Outdated
hodlbod
commented
aaaaa aaaaa
mplorentz
commented
haha, yes. Sorry I did do a self-review of this code but clearly I missed this function or my eyes glazed over while reading it. Cleaning it up. haha, yes. Sorry I did do a self-review of this code but clearly I missed this function or my eyes glazed over while reading it. Cleaning it up.
|
||||
throw e
|
||||
}
|
||||
|
||||
participantPubkeyMap.set(new Map())
|
||||
addParticipant(room.localParticipant.identity)
|
||||
for (const p of room.remoteParticipants.values()) {
|
||||
addParticipant(p.identity)
|
||||
}
|
||||
|
||||
let muted = false
|
||||
try {
|
||||
await room.localParticipant.setMicrophoneEnabled(true)
|
||||
} catch (e) {
|
||||
muted = true
|
||||
pushToast({theme: "error", message: "Could not access microphone"})
|
||||
}
|
||||
|
||||
currentVoiceSession.set({url, h, room, muted})
|
||||
voiceState.set("connected")
|
||||
playJoinSound()
|
||||
} catch (e) {
|
||||
room.disconnect()
|
||||
if (isActive()) voiceState.set("disconnected")
|
||||
if (e instanceof AbortError) return
|
||||
throw e
|
||||
} finally {
|
||||
if (isActive()) joinAbortController = undefined
|
||||
}
|
||||
|
||||
participantPubkeyMap.set(new Map())
|
||||
addParticipant(room.localParticipant.identity)
|
||||
for (const p of room.remoteParticipants.values()) {
|
||||
addParticipant(p.identity)
|
||||
}
|
||||
|
||||
let muted = false
|
||||
try {
|
||||
await room.localParticipant.setMicrophoneEnabled(true)
|
||||
} catch (e) {
|
||||
muted = true
|
||||
pushToast({theme: "error", message: "Could not access microphone"})
|
||||
}
|
||||
|
||||
currentVoiceSession.set({url, h, room, muted})
|
||||
|
||||
playJoinSound()
|
||||
}
|
||||
|
||||
export const leaveVoiceRoom = async () => {
|
||||
@@ -236,10 +259,16 @@ export const leaveVoiceRoom = async () => {
|
||||
|
||||
speakingParticipants.set([])
|
||||
participantPubkeyMap.set(new Map())
|
||||
voiceState.set("disconnected")
|
||||
session.room.disconnect()
|
||||
currentVoiceSession.set(undefined)
|
||||
}
|
||||
|
||||
export const rejoinVoiceRoom = () => {
|
||||
const target = get(currentVoiceRoom)
|
||||
if (target) joinVoiceRoom(target.url, target.h)
|
||||
}
|
||||
|
||||
export const toggleMute = async () => {
|
||||
const session = get(currentVoiceSession)
|
||||
if (!session) return
|
||||
|
||||
@@ -42,13 +42,15 @@
|
||||
decodeRelay,
|
||||
deriveRoom,
|
||||
deriveUserRoomMembershipStatus,
|
||||
getRoomType,
|
||||
MESSAGE_KINDS,
|
||||
MembershipStatus,
|
||||
PROTECTED,
|
||||
RoomType,
|
||||
userSettingsValues,
|
||||
} from "@app/core/state"
|
||||
import VoiceWidget from "@app/components/VoiceWidget.svelte"
|
||||
import {currentVoiceSession} from "@app/voice"
|
||||
import {voiceState} from "@app/voice"
|
||||
import {makeFeed} from "@app/core/requests"
|
||||
import {popKey} from "@lib/implicit"
|
||||
import {checked} from "@app/util/notifications"
|
||||
@@ -60,6 +62,7 @@
|
||||
const lastChecked = $checked[$page.url.pathname]
|
||||
const url = decodeRelay(relay)
|
||||
const room = deriveRoom(url, h)
|
||||
const isVoiceRoom = $derived(getRoomType($room) === RoomType.Voice)
|
||||
const shouldProtect = canEnforceNip70(url)
|
||||
const membershipStatus = deriveUserRoomMembershipStatus(url, h)
|
||||
const at = $derived(parseInt($page.url.searchParams.get("at")!))
|
||||
@@ -497,7 +500,7 @@
|
||||
{/key}
|
||||
{/if}
|
||||
</div>
|
||||
{#if $currentVoiceSession}
|
||||
{#if isVoiceRoom || $voiceState === "joining" || $voiceState === "connected"}
|
||||
<div class="hide-on-keyboard flex-shrink-0 p-2 md:hidden">
|
||||
<VoiceWidget />
|
||||
</div>
|
||||
|
||||
deriveProfileis designed to acceptundefinedto avoid this kind of mess