coracle/flotilla
1
3
Fork 9
Code Issues 32 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
+39 -1
1 changed files with 39 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 140440ca4c - Show all commits
src/app/components/RoomForm.svelte
+39 -1
View File
@@ -16,6 +16,24 @@
import {pushToast} from "@app/util/toast"
import {uploadFile} from "@app/core/commands"
type RoomMode = "text" | "voice" | "both"
const getRoomModeFromEvent = (event?: {tags?: string[][]}): RoomMode => {
const tags = event?.tags ?? []
const hasLivekit = tags.some(t => t[0] === "livekit")
const hasNoText = tags.some(t => t[0] === "no-text")
if (hasLivekit && hasNoText) return "voice"
if (hasLivekit) return "both"
return "text"
}
hodlbod marked this conversation as resolved Outdated
hodlbod commented 2026-03-03 23:14:14 +00:00
Outdated
Review
Copy Link
Copy Source

[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
mplorentz commented 2026-03-04 19:09:39 +00:00
Outdated
Review
Copy Link
Copy Source

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?
hodlbod commented 2026-03-04 19:43:34 +00:00
Outdated
Review
Copy Link
Copy Source

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

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)`?
hodlbod commented 2026-03-05 16:01:37 +00:00
Outdated
Review
Copy Link
Copy Source

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
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"]]
return filtered
}
hodlbod marked this conversation as resolved Outdated
hodlbod commented 2026-03-03 23:12:52 +00:00
Outdated
Review
Copy Link
Copy Source

[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)) ```
type Props = {
url: string
header: Snippet
@@ -27,12 +45,16 @@
const {url, header, footer, onsubmit, initialValues = makeRoomMeta()}: Props = $props()
const values = $state(initialValues)
let roomMode = $state<RoomMode>(getRoomModeFromEvent(initialValues.event))
const submit = async () => {
const room = $state.snapshot(values)
if (imageFile) {
const {error, result} = await uploadFile(imageFile, {maxWidth: 256, maxHeight: 256})
const {error, result} = await uploadFile(imageFile, {
maxWidth: 256,
maxHeight: 256,
})
if (error) {
return pushToast({theme: "error", message: error})
1
 @@ -42,6 +64,10 @@
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))
hodlbod marked this conversation as resolved Outdated
hodlbod commented 2026-03-03 23:16:13 +00:00
Outdated
Review
Copy Link
Copy Source

This should never run if we make the form change below.

This should never run if we make the form change below.
if (createMessage && !createMessage.includes("already")) {
4
 @@ -178,6 +204,18 @@
<input type="checkbox" class="checkbox" bind:checked={values.isClosed} />
<span class="text-sm opacity-75">Ignore requests to join</span>
</div>
<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>
</ModalBody>
{@render footer({loading})}
</Modal>
hodlbod marked this conversation as resolved Outdated
hodlbod commented 2026-03-03 23:14:48 +00:00
Outdated
Review
Copy Link
Copy Source

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
mplorentz commented 2026-03-04 20:19:35 +00:00
Outdated
Review
Copy Link
Copy Source

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.
hodlbod commented 2026-03-04 21:08:12 +00:00
Outdated
Review
Copy Link
Copy Source

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?"