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
4 changed files with 65 additions and 61 deletions
Showing only changes of commit f4e4ca1038 - Show all commits
+34 -40
View File
@@ -1,7 +1,9 @@
<script lang="ts">
import {get} from "svelte/store"
import type {Snippet} from "svelte"
import {assoc, append, nth, type Maybe, uniqBy} from "@welshman/lib"
import type {RoomMeta} from "@welshman/util"
import {makeRoomMeta} from "@welshman/util"
import {getTag, makeRoomMeta} from "@welshman/util"
import {waitForThunkError, createRoom, editRoom, joinRoom} from "@welshman/app"
import StickerSmileSquare from "@assets/icons/sticker-smile-square.svg?dataurl"
import Hashtag from "@assets/icons/hashtag.svg?dataurl"
@@ -15,14 +17,14 @@
import ModalBody from "@lib/components/ModalBody.svelte"
import {pushToast} from "@app/util/toast"
import {uploadFile} from "@app/core/commands"
import {checkRelayHasLivekit} from "@app/voice"
import {deriveHasLivekit} from "@app/core/state"
type RoomMode = "text" | "voice" | "both"
const getRoomModeFromEvent = (event?: {tags?: string[][]}): RoomMode => {
const getRoomModeFromEvent = (event: Maybe<{tags: Maybe<string[][]>}>): RoomMode => {
const tags = event?.tags ?? []
const hasLivekit = tags.some(t => t[0] === "livekit")
const hasNoText = tags.some(t => t[0] === "no-text")
const hasLivekit = !!getTag("livekit", tags)
const hasNoText = !!getTag("no-text", tags)
if (hasLivekit && hasNoText) return "voice"
hodlbod marked this conversation as resolved Outdated
Outdated
Review

[nit] A useful util is getTagValue("livekit", tags) just for clarity. Also, beef up the type signature, either pass maybe or pass a TrustedEvent

[nit] A useful util is `getTagValue("livekit", tags)` just for clarity. Also, beef up the type signature, either pass maybe<tags> or pass a TrustedEvent
Outdated
Review

Ok cool. Can you explain why you prefer Maybe over typescripts built-in optional type? Is that a pretty universal preference for you?

Ok cool. Can you explain why you prefer Maybe over typescripts built-in optional type? Is that a pretty universal preference for you?
Outdated
Review

The optional type means scattering around a bunch of ad-hoc types that would need to be updated if the model changes (not that it will here, but it's still redundant). (event?: TrustedEvent) is stronger and allows more assumptions about what's actually happening. If you just want to accept tags so that (for example) callers can call this without having an event, you could do (tags: string[][] = []). But this function should always be able to assume that an event exists

The optional type means scattering around a bunch of ad-hoc types that would need to be updated if the model changes (not that it will here, but it's still redundant). `(event?: TrustedEvent)` is stronger and allows more assumptions about what's actually happening. If you just want to accept tags so that (for example) callers can call this without having an event, you could do `(tags: string[][] = [])`. But this function should always be able to assume that an event exists
Outdated
Review

Sorry, I'm asking why you suggested (event: Maybe<TrustedEvent>) over (event?: TrustedEvent)?

Sorry, I'm asking why you suggested `(event: Maybe<TrustedEvent>)` over `(event?: TrustedEvent)`?
Outdated
Review

Oh, I see, no optional parameters are better, I was just being vague

Oh, I see, no optional parameters are better, I was just being vague
if (hasLivekit) return "both"
return "text"
@@ -30,8 +32,9 @@
const buildTagsWithRoomMode = (existingTags: string[][], roomMode: RoomMode): string[][] => {
const filtered = existingTags.filter(t => t[0] !== "livekit" && t[0] !== "no-text")
if (roomMode === "both") return [...filtered, ["livekit"]]
if (roomMode === "voice") return [...filtered, ["livekit"], ["no-text"]]
if (roomMode === "both") return uniqBy(nth(0), append(["livekit"], filtered))
hodlbod marked this conversation as resolved Outdated
Outdated
Review

[nit]

Stylistically I normally do something like:

return uniqBy(nth(0), append(["livekit"], tags))
[nit] Stylistically I normally do something like: ``` return uniqBy(nth(0), append(["livekit"], tags)) ```
if (roomMode === "voice")
return uniqBy(nth(0), append(["no-text"], append(["livekit"], filtered)))
return filtered
}
@@ -47,23 +50,12 @@
const values = $state(initialValues)
let roomMode = $state<RoomMode>(getRoomModeFromEvent(initialValues.event))
let relayHasLivekit = $state<boolean | undefined>(undefined)
$effect(() => {
const u = url
let cancelled = false
checkRelayHasLivekit(u).then(has => {
if (!cancelled) relayHasLivekit = has
})
return () => {
cancelled = true
}
})
const relayHasLivekit = deriveHasLivekit(url)
const submit = async () => {
const room = $state.snapshot(values)
if ((roomMode === "voice" || roomMode === "both") && !relayHasLivekit) {
if ((roomMode === "voice" || roomMode === "both") && !get(relayHasLivekit)) {
return pushToast({
theme: "error",
message: "This relay does not support voice rooms.",
hodlbod marked this conversation as resolved Outdated
Outdated
Review

Better would be to add a util to app/state called deriveHasLiveKit(url) that can cache the request. This complex effect behavior is something I've noticed a lot recently, but we can assume url will not change here.

Better would be to add a util to app/state called `deriveHasLiveKit(url)` that can cache the request. This complex effect behavior is something I've noticed a lot recently, but we can assume url will not change here.
1
@@ -84,16 +76,17 @@
room.pictureMeta = result.tags
}
const existingTags = room.event?.tags ?? []
const tags = buildTagsWithRoomMode(existingTags, roomMode)
room.event = room.event ? {...room.event, tags} : ({tags} as RoomMeta["event"])
const createMessage = await waitForThunkError(createRoom(url, room))
if (createMessage && !createMessage.includes("already")) {
return pushToast({theme: "error", message: createMessage})
}
if (room.event && get(relayHasLivekit)) {
const existingTags = room.event.tags ?? []
const tags = buildTagsWithRoomMode(existingTags, roomMode)
room.event = assoc("tags", tags)(room.event) as RoomMeta["event"]
}
Outdated
Review

createRoom doesn't use the event property (because it shouldn't exist yet). I think it's probably better to include this tag on the room update event anyhow.

`createRoom` doesn't use the event property (because it shouldn't exist yet). I think it's probably better to include this tag on the room update event anyhow.
Outdated
Review

Ah, ok. I think you mentioned including the tag on the update event in a comment on the proposed NIP too. I was very confused but now I see that you always try to publish a kind 9007 room create and then immediately a 9002 room metadata edit. Why? Can't you just put all the tags in the 9007? Do all the NIP-29 clients behave this way?

Related question: do I remember correctly that you use VSCode? Do you have some trick for being able to command-click into Welshman functions from the flotilla repo? I have tried and failed to get Cursor to index those functions. Alt-tabbing and ctrl-fing to read welshman function implementations is pretty annoying.

Ah, ok. I think you mentioned including the tag on the update event in a comment on the proposed NIP too. I was very confused but now I see that you always try to publish a kind 9007 room create and then immediately a 9002 room metadata edit. Why? Can't you just put all the tags in the 9007? Do all the NIP-29 clients behave this way? Related question: do I remember correctly that you use VSCode? Do you have some trick for being able to command-click into Welshman functions from the flotilla repo? I have tried and failed to get Cursor to index those functions. Alt-tabbing and ctrl-fing to read welshman function implementations is pretty annoying.
Outdated
Review

Just the way the spec was written, I agree it's stupid. Most relays will reject meta kinds on 9007, so both events are necessary.

No haha I use kakoune and do everything manually. Intellisense is for the weak. If you're able to fix it go right ahead, there is some squirreliness somewhere in there, sourcemaps don't load either.

Just the way the spec was written, I agree it's stupid. Most relays will reject meta kinds on 9007, so both events are necessary. No haha I use kakoune and do everything manually. Intellisense is for the weak. If you're able to fix it go right ahead, there is some squirreliness somewhere in there, sourcemaps don't load either.
Outdated
Review

This line is still a hack, since it circumvents the welshman data model. I went ahead and released welshman 0.8.8 which adds support for room.livekit and room.noText booleans.

This line is still a hack, since it circumvents the welshman data model. I went ahead and released welshman 0.8.8 which adds support for room.livekit and room.noText booleans.
const editMessage = await waitForThunkError(editRoom(url, room))
if (editMessage) {
@@ -207,22 +200,23 @@
</label>
{/snippet}
</FieldInline>
<FieldInline>
{#snippet label()}
<p>Room type</p>
{/snippet}
{#snippet input()}
<select class="select select-bordered w-full" bind:value={roomMode} aria-label="Room type">
<option value="text">Text only</option>
<option value="both" disabled={relayHasLivekit === false}>
Text and voice{relayHasLivekit === false ? " (not setup)" : ""}
</option>
<option value="voice" disabled={relayHasLivekit === false}>
Voice only{relayHasLivekit === false ? " (not setup)" : ""}
</option>
</select>
{/snippet}
</FieldInline>
{#if $relayHasLivekit}
<FieldInline>
{#snippet label()}
<p>Room type</p>
{/snippet}
{#snippet input()}
<select
class="select select-bordered w-full"
bind:value={roomMode}
aria-label="Room type">
<option value="text">Text only</option>
<option value="both">Text and voice</option>
<option value="voice">Voice only</option>
</select>
{/snippet}
</FieldInline>
{/if}
<strong class="md:hidden">Permissions</strong>
<div class="flex items-center gap-2">
hodlbod marked this conversation as resolved Outdated
Outdated
Review

Instead of showing (not setup) let's only show this field if the relay has livekit support

Instead of showing (not setup) let's only show this field if the relay has livekit support
Outdated
Review

I will do that, but imo it makes some pretty bad UX if the relay loses livekit support. Either we silently convert voice or voice + text rooms to text only on edit, which may not be what the user wants. Or we don't silently convert and we leave them no way to do the conversion themselves.

I will do that, but imo it makes some pretty bad UX if the relay loses livekit support. Either we silently convert voice or voice + text rooms to text only on edit, which may not be what the user wants. Or we don't silently convert and we leave them no way to do the conversion themselves.
Outdated
Review

Interesting point. The general approach I take for feature-detected stuff is to hide it if it's not supported to keep the UI from being unnecessarily complex. For this, maybe we could swap out the select input for a warning that says "This room requires voice support which isn't currently provided by myrelay.example.com. Convert to a text-only room?" But that seems like an edge case better handled by the administrator. "Why don't my voice rooms work anymore?" is a much more likely question than "Why did my room change to text-only on edit?"

Interesting point. The general approach I take for feature-detected stuff is to hide it if it's not supported to keep the UI from being unnecessarily complex. For this, maybe we could swap out the select input for a warning that says "This room requires voice support which isn't currently provided by myrelay.example.com. Convert to a text-only room?" But that seems like an edge case better handled by the administrator. "Why don't my voice rooms work anymore?" is a much more likely question than "Why did my room change to text-only on edit?"
<input type="checkbox" class="checkbox" bind:checked={values.isRestricted} />
+7
View File
@@ -146,6 +146,7 @@ import {
displayProfileByPubkey,
getProfile,
} from "@welshman/app"
import {checkRelayHasLivekit} from "$lib/livekit"
import {readFeed} from "@lib/feeds"
export const fromCsv = (s: string) => (s || "").split(",").filter(identity)
4
@@ -1197,6 +1198,12 @@ export const deriveSupportedMethods = simpleCache(([url]: [string]) => {
})
})
export const deriveHasLivekit = simpleCache(([url]: [string]) =>
readable<boolean | undefined>(undefined, set => {
checkRelayHasLivekit(url).then(has => set(has))
}),
)
export const deriveTimeout = (timeout: number) => {
const store = writable<boolean>(false)
+3 -21
View File
@@ -8,31 +8,13 @@ import {derived, get, writable} from "svelte/store"
import {now} from "@welshman/lib"
import {makeEvent, getTagValue} from "@welshman/util"
import {signer, publishThunk} from "@welshman/app"
import {getLivekitEndpoint} from "$lib/livekit"
import {deriveEventsForUrl} from "@app/core/state"
import {pushToast} from "@app/util/toast"
export const ROOM_PRESENCE = 10312
const livekitEndpoint = (url: string, groupId: string) => {
const httpUrl = url
.replace(/^wss:\/\//, "https://")
.replace(/^ws:\/\//, "http://")
.replace(/\/$/, "")
return `${httpUrl}/.well-known/nip29/livekit/${groupId}`
}
export const checkRelayHasLivekit = async (url: string): Promise<boolean> => {
const endpoint = livekitEndpoint(url, "nop")
try {
// Currently we are hitting the API with no auth because zooid returns a 401 livekit
// is configured and 404 if it is not. But we need a standardized solution in the NIP.
const response = await fetch(endpoint)
return response.status === 401
} catch {
return false
}
}
export {checkRelayHasLivekit} from "$lib/livekit"
const PRESENCE_INTERVAL_MS = 60_000
const PRESENCE_EXPIRY_S = 300
@@ -53,7 +35,7 @@ const fetchLivekitToken = async (
groupId: string,
signal?: AbortSignal,
): Promise<{server_url: string; participant_token: string}> => {
const endpoint = livekitEndpoint(url, groupId)
const endpoint = getLivekitEndpoint(url, groupId)
const $signer = signer.get()
if (!$signer) throw new Error("No signer available")
8
+21
View File
@@ -0,0 +1,21 @@
const livekitEndpoint = (url: string, groupId: string) => {
const httpUrl = url
.replace(/^wss:\/\//, "https://")
.replace(/^ws:\/\//, "http://")
.replace(/\/$/, "")
return `${httpUrl}/.well-known/nip29/livekit/${groupId}`
}
export const checkRelayHasLivekit = async (url: string): Promise<boolean> => {
const endpoint = livekitEndpoint(url, "nop")
try {
// Zooid returns 401 when livekit is configured and 404 if it is not.
const response = await fetch(endpoint)
return response.status === 401
} catch {
return false
}
}
export const getLivekitEndpoint = (url: string, groupId: string) => livekitEndpoint(url, groupId)