coracle/flotilla
1
3
Fork 9
Code Issues 31 Pull Requests 1 Actions Packages Projects 5 Releases Wiki Activity

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
Conversation 28 Commits 68 Files Changed 28
+59 -12
2 changed files with 59 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 559df4b948 - Show all commits
src/app/components/VoiceRoomItem.svelte
+21 -7
View File
@@ -6,7 +6,12 @@
import ProfileCircle from "@app/components/ProfileCircle.svelte"
import RoomName from "@app/components/RoomName.svelte"
import {pushToast} from "@app/util/toast"
import {deriveVoiceParticipants, joinVoiceRoom, currentVoiceSession} from "@app/voice"
import {
deriveVoiceParticipants,
joinVoiceRoom,
leaveVoiceRoom,
currentVoiceSession,
} from "@app/voice"
interface Props {
url: string
@@ -18,17 +23,29 @@
const participants = deriveVoiceParticipants(url, h)
const isActive = $derived($currentVoiceSession?.url === url && $currentVoiceSession?.h === h)
let isJoining = $state(false)
let joinAbortController: AbortController | undefined
const handleClick = async () => {
if (isJoining) return
if (isActive) {
await leaveVoiceRoom()
return
}
if (isJoining) {
joinAbortController?.abort()
return
hodlbod marked this conversation as resolved Outdated
hodlbod commented 2026-03-05 22:40:15 +00:00
Outdated
Review
Copy Link
Copy Source

Instead of this, we should just not render voice rooms on mobile

Instead of this, we should just not render voice rooms on mobile
👍 1
mplorentz commented 2026-03-06 16:02:20 +00:00
Outdated
Review
Copy Link
Copy Source

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.
}
joinAbortController = new AbortController()
isJoining = true
try {
await joinVoiceRoom(url, h)
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}`})
hodlbod marked this conversation as resolved Outdated
hodlbod commented 2026-03-05 22:41:57 +00:00
Outdated
Review
Copy Link
Copy Source

[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
👍 1
} finally {
isJoining = false
joinAbortController = undefined
}
}
hodlbod marked this conversation as resolved Outdated
hodlbod commented 2026-03-03 23:25:00 +00:00
Outdated
Review
Copy Link
Copy Source

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.
mplorentz commented 2026-03-05 15:26:49 +00:00
Outdated
Review
Copy Link
Copy Source

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?
mplorentz commented 2026-03-05 15:30:05 +00:00
Outdated
Review
Copy Link
Copy Source

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 😓
mplorentz commented 2026-03-05 15:34:34 +00:00
Outdated
Review
Copy Link
Copy Source

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.
hodlbod commented 2026-03-05 16:09:28 +00:00
Outdated
Review
Copy Link
Copy Source

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.
@@ -40,10 +57,7 @@
</script>
<div>
<SecondaryNavItem
onclick={handleClick}
disabled={isJoining}
class={isActive ? "!bg-base-100 !text-base-content" : ""}>
<SecondaryNavItem onclick={handleClick} class={isActive ? "!bg-base-100 !text-base-content" : ""}>
{#if isJoining}
<span class="loading loading-spinner loading-sm"></span>
{:else}
2
src/app/voice.ts
+38 -5
View File
@@ -24,6 +24,7 @@ export const currentVoiceSession = writable<VoiceSession | undefined>(undefined)
const fetchLivekitToken = async (
url: string,
groupId: string,
signal?: AbortSignal,
): Promise<{server_url: string; participant_token: string}> => {
const httpUrl = url
.replace(/^wss:\/\//, "https://")
@@ -34,6 +35,8 @@ const fetchLivekitToken = async (
const $signer = signer.get()
if (!$signer) throw new Error("No signer available")
if (signal?.aborted) throw new Error("Join cancelled")
const authHeader = await getToken(
endpoint,
"GET",
1
 @@ -47,9 +50,16 @@ const fetchLivekitToken = async (
true,
)
const response = await fetch(endpoint, {
headers: {Authorization: authHeader},
})
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
}
hodlbod marked this conversation as resolved Outdated
hodlbod commented 2026-03-03 23:38:26 +00:00
Outdated
Review
Copy Link
Copy Source

Use welshman's version instead (makeHttpAuth/makeHttpAuthHeader)

Use welshman's version instead (makeHttpAuth/makeHttpAuthHeader)
if (!response.ok) {
const text = await response.text()
2
 @@ -104,7 +114,11 @@ const stopPresenceHeartbeat = () => {
}
}
export const joinVoiceRoom = async (url: string, h: string) => {
export const joinVoiceRoom = async (
url: string,
h: string,
signal?: AbortSignal,
): Promise<void> => {
const session = get(currentVoiceSession)
if (session) {
@@ -112,7 +126,9 @@ export const joinVoiceRoom = async (url: string, h: string) => {
await leaveVoiceRoom()
}
const {server_url, participant_token} = await fetchLivekitToken(url, h)
const {server_url, participant_token} = await fetchLivekitToken(url, h, signal)
if (signal?.aborted) throw new Error("Join cancelled")
const room = new Room({
adaptiveStream: true,
@@ -144,6 +160,17 @@ export const joinVoiceRoom = async (url: string, h: string) => {
track.detach().forEach(el => el.remove())
})
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
try {
@@ -158,8 +185,14 @@ export const joinVoiceRoom = async (url: string, h: string) => {
])
} catch (e) {
room.disconnect()
if (signal?.aborted) {
throw new Error("Join cancelled")
}
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})
hodlbod marked this conversation as resolved Outdated
hodlbod commented 2026-03-03 23:45:26 +00:00
Outdated
Review
Copy Link
Copy Source

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.
👍 1
1